fix(codex): wrap token-store JSON parse errors and use atomic write (#128)#162
Merged
fix(codex): wrap token-store JSON parse errors and use atomic write (#128)#162
Conversation
Contributor
There was a problem hiding this comment.
Findings
- [Major] PID-only tmp path can break atomic writes under concurrent calls —
write()now always targets${filePath}.tmp.${pid}. Two concurrent writes in the same process can race on the same tmp file (overwrite/unlink each other), causing failed or lost credential persistence, evidencepackages/providers/src/codex/token-store.ts:98.
Suggested fix:import { randomUUID } from 'node:crypto'; const tmpPath = `${this.filePath}.tmp.${process.pid}.${randomUUID()}`;
Summary
- Review mode: initial
- 1 issue found in fresh diff.
docs/VISION.md/docs/PRINCIPLES.mdare Not found in repo/docs in this checkout, so this review usedCLAUDE.md+ changed source files only.
open-codesign Bot
Testing
- Not run (automation)
- Suggested tests: add a concurrent
Promise.all([store.write(a), store.write(b)])case asserting no.tmp.*leftovers and final file JSON is valid.
| // Write to a pid-scoped tmp then atomically rename. Guards against | ||
| // truncated writes on Win11 when the process is killed or antivirus | ||
| // interferes mid-write (issue #128). | ||
| const tmpPath = `${this.filePath}.tmp.${process.pid}`; |
Contributor
There was a problem hiding this comment.
Using only process.pid in tmpPath makes concurrent writes in the same process collide on one temp file. Please make temp names unique per write (for example add randomUUID()), so atomicity still holds when write() is called concurrently.
import { randomUUID } from 'node:crypto';
const tmpPath = `${this.filePath}.tmp.${process.pid}.${randomUUID()}`;…128) Win11 generation could fail with a bare `Unexpected end of JSON input` when the Codex OAuth token file on disk was truncated. Root cause was twofold: `read()` threw plain `Error` on JSON parse/schema failure (IPC surfaced only `SyntaxError.message` with no code to classify), and `write()` was a direct `writeFile` that could leave a half-written file if the process was killed or antivirus locked the handle mid-write. - token-store `read()` now throws `CodesignError(CODEX_TOKEN_PARSE_FAILED)` with the original `SyntaxError` as `cause`, so diagnostics can classify it as a provider error and show the localized "re-login" hint. - token-store `write()` now writes to `${path}.tmp.${pid}` and renames atomically; the tmp file is unlinked on failure so a crash mid-write cannot leave the destination truncated. - "not logged in" paths throw `CodesignError(CODEX_TOKEN_NOT_LOGGED_IN)` so the same diagnostic pipeline can route them. - Added error codes + i18n keys (en / zh-CN) for both conditions. - Test coverage: parse-failed on truncated JSON and on invalid schema, not-logged-in on missing file, atomic write leaves no tmp and does not clobber the destination when rename fails. Signed-off-by: hqhq1025 <1506751656@qq.com>
4da2eeb to
1bd3b08
Compare
Contributor
There was a problem hiding this comment.
Findings
- [Major] 原子写临时文件名只使用
process.pid,同一进程内并发调用write()会竞争同一个tmp文件,可能互相覆盖/删除并导致凭证写入失败或丢失,证据packages/providers/src/codex/token-store.ts:98。
Suggested fix:import { randomUUID } from 'node:crypto'; const tmpPath = `${this.filePath}.tmp.${process.pid}.${randomUUID()}`;
Summary
- Review mode: follow-up after new commits
- 发现 1 个问题(以上)。
docs/VISION.md、docs/PRINCIPLES.md:Not found in repo/docs(本次仓库副本中不存在)。
Testing
- Not run (automation)
- 建议补充:
Promise.all([store.write(a), store.write(b)])并发写测试,断言最终 JSON 合法且无.tmp.*残留。
open-codesign Bot
| // Write to a pid-scoped tmp then atomically rename. Guards against | ||
| // truncated writes on Win11 when the process is killed or antivirus | ||
| // interferes mid-write (issue #128). | ||
| const tmpPath = `${this.filePath}.tmp.${process.pid}`; |
Contributor
There was a problem hiding this comment.
tmpPath 只包含 process.pid 时,同进程并发写会撞名,导致原子写退化(覆盖/误删同一 tmp)。建议给每次写入增加唯一后缀(例如 randomUUID()):
import { randomUUID } from 'node:crypto';
const tmpPath = `${this.filePath}.tmp.${process.pid}.${randomUUID()}`;…ocess races (#128) Signed-off-by: hqhq1025 <1506751656@qq.com>
Contributor
There was a problem hiding this comment.
Findings
- 未发现达到报告阈值(>=80% 置信度)的新增问题。
Summary
Review mode: follow-up after new commits
- 本次 follow-up 提交已修复上一轮指出的并发
tmpPath冲突问题:packages/providers/src/codex/token-store.ts:102使用randomUUID(),并新增并发写回归测试packages/providers/src/codex/token-store.test.ts:326。 - 约束检查(本次变更范围内):未发现直接 provider SDK 引入、未发现 silent fallback、新增错误码已在共享错误码表登记。
docs/VISION.md、docs/PRINCIPLES.md: Not found in repo/docs(当前仓库副本中不存在)。- 残余风险:未在本次自动化中复跑全量 CI,仅基于 diff 与测试代码静态审阅。
Testing
- Not run (automation)
open-codesign Bot
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Win11 用户偶发 `Error invoking remote method 'codesign:v1:generate': CodesignError: Unexpected end of JSON input`。根因:`codex-auth.json` 被进程被 kill / antivirus 干扰 truncate 后,`token-store.read()` 抛裸 `Error`,IPC 透传时只剩 `SyntaxError.message`,用户完全不知所措。
What changed
Test plan
Closes #128