Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: luvs01/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Resolved — PR description corrected. |
The suffix cursor was keyed by the full sanitized base, but a candidate keeps at most MAX_TOOL_CALL_ID_LENGTH - 2 characters of it — longer suffixes only truncate deeper. Distinct ids agreeing on that prefix therefore produce the same candidate sequence while each restarting the search at -2, so M prefix-sharing ids emitted twice still cost ~M^2/2 occupied-set probes. Key the cursor by the post-truncation prefix — the actual collision domain — so all ids sharing it resume the same search. The occupied-set check still decides acceptance, keeping emitted ids unique and within the 64-char bound. Regression test: 1,000 conforming 64-char ids plus 500 overlength non-conforming ids, all sharing the first 62 characters and each emitted twice, stay under 10k probes (was ~1.13M before). Co-Authored-By: Epinephrine <luvs01@hanmail.net>
0b09686 to
ccfb8e6
Compare
|
이관됨: lidge-jun#6083 |
|
현재 dev 기준 리베이스 후 상류 제출 완료. 포크 PR 목적 달성으로 종료. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccfb8e63e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // prefix is the collision domain. The resume cursor has to live on the domain, not the full | ||
| // base: distinct ids sharing it would otherwise each restart at -2 and re-run every probe an | ||
| // earlier base already made, which is quadratic in the number of such ids. | ||
| const prefix = base.slice(0, MAX_TOOL_CALL_ID_LENGTH - 2); |
There was a problem hiding this comment.
Fresh evidence beyond the earlier report is the next suffix-width boundary: IDs that share only the first 60 characters have different 62-character map keys here, but once n >= 100, line 40 truncates every candidate to those same 60 characters. Each domain therefore re-probes the candidates occupied by earlier domains; for example, 1,024 distinct 62-character IDs with a common 60-character prefix, each repeated 100 times, cause about 94.7 million Set.has probes for roughly 103,000 calls. Thus an attacker-controlled response remains quadratic despite the documented linear contract. Coordinate progress by the prefix retained at each suffix width, or add a fixed base discriminator so candidate sequences cannot converge.
AGENTS.md reference: structure/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
1 participant
Motivation
openai-chatupstream could emit many duplicate tool-call IDs and cause the reminter to restart suffix enumeration at-2for each duplicate, producing O(N²)Set.hasprobes and stalling the event loop.Description
createToolCallIdReminterresume suffix search per truncated collision domain by keyingnextSuffixByBase: Map<string, number>onbase.slice(0, MAX_TOOL_CALL_ID_LENGTH - 2)— the post-truncation prefix that determines the candidate sequence — and updating it when a candidate is chosen insrc/adapters/openai-chat/tool-call-id-remint.ts.rawIdremints and asserts the number ofSet.prototype.hasprobes stays bounded, intests/adapters/openai/openai-chat-tool-call-id-remint.test.ts.structure/providers-and-adapters.md.Testing
bun test tests/adapters/openai/openai-chat-tool-call-id-remint.test.ts, which passed (14 passed, 0 failed).bun run typecheck, which succeeded.bun run structure:checkandbun run privacy:scan, both of which succeeded.bun run test:changedcould not be executed in this checkout because the comparison ref (dev/remote) is not available; other focused validation above was performed instead.Codex Task