fix: exclude same-millisecond replayed usage from scan budgets - #631
fix: exclude same-millisecond replayed usage from scan budgets#631mldangelo-oai wants to merge 40 commits into
Conversation
|
@codex review |
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. |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
kmbroai
left a comment
kmbroai
left a comment
There was a problem hiding this comment.
Critical review
Reviewed cb3e851450b603fddecdb1d8fe0a72f07285f9a3, against its declared #630 base. The same-millisecond attribution fix is justified; no blocking correctness issue found.
Necessity and correctness
Counting replayed parent usage as new child usage can terminate a scan against a user-requested cost limit. Comparing only the UUIDv7 timestamp cannot distinguish two IDs created within one millisecond. The change keeps the full ordering in both the live tracker and the bundled collector, so those paths agree about the inherited baseline. BigInt in TypeScript avoids losing the low ordering bits through Number conversion.
There is an important compatibility assumption: UUIDv7 format alone does not promise ordering within a millisecond. This relies on the producer's monotonic generator. I checked current public upstream source: Codex thread IDs and submission IDs use Uuid::now_v7, whose documented guarantee is creation order within the same process. Keep that producer relationship explicit; do not broaden this comparison to unrelated imported UUIDs or assume it proves cross-process chronology. This was a current-source check, not a rebuild of the pinned Codex runtime.
Simplification and scope
The production change is small and appropriately avoids a new attribution ledger. Keep the existing non-UUID fallback rather than adding arbitrary ID rejection. The bundle-version change is necessary for the Python collector to reach cached installations; coordinate it with the #623 → #626 → #630 stack rather than treating this as an independent version bump on main.
The test setup is much larger than the logic. The lower/higher same-millisecond controls and cross-runtime usage comparison are useful. The mock marketplace installer and exact bundle-version expectations would fit better in the existing runtime-upgrade fixture than being duplicated inside cost tests. The CODEX_SAFETY_IDENTIFIER assertion is unrelated to replay attribution and can stay in the package/environment tests. Preserve the installed-collector check, but reuse the existing setup where possible.
Verification
Ran cost.test.ts and diff-rank-input.test.ts with seed 12345: 64 passed, 0 failed, Bun 1.3.14/Linux with locally available dependencies. This includes the cached-collector and cross-runtime controls. No real billing, live model call, full clean install, or native platform matrix was exercised.
Keep the confinement bundle distinct from its updated parent and verify the predecessor cache upgrade.
|
@codex review |
|
@codex security review |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed adb12010bf4676cb9ff9eb0c78fdb56d42a39ac2 against declared base 446ff8fdbe08bdcb9a341a25fe79c867f1ed9e03 (#630). No blocking concern in this PR's replay-attribution delta; its own patch is unchanged from the previously reviewed restack.
The 67 focused cost/diff-input/cache tests and incremental formatting, syntax, version, and diff checks passed on this head. Full current-head node-ci 32812126779, attempt 2 now passes all 25 jobs, including native Windows. Head, base, CI, and discussions were rechecked before approval; no unchanged local suite was rerun for this CI-only follow-up. My earlier CI-hold comment concerned 5483c74b, not this head.
Approval covers only this PR's delta, not its parents. The existing within-process monotonic UUIDv7 qualification remains; upstream producer behavior and live billing/model execution were not revalidated.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7afffc779
ℹ️ 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".
| const threadOrder = uuid7Order(session.threadId); | ||
| const turnOrder = uuid7Order(payload["turn_id"]); | ||
| const owned = | ||
| threadOrder === null | ||
| ? typeof payload["started_at"] === "number" && | ||
| session.startedAt !== null && | ||
| payload["started_at"] >= Math.floor(session.startedAt / 1_000) | ||
| : turnOrder !== null && turnOrder >= threadOrder; |
There was a problem hiding this comment.
When a child thread and its first turn share a millisecond, UUIDv7 does not generally guarantee that the random tail of the later-created turn is greater than the thread's tail. A genuine lower-valued turn therefore leaves a one-turn worker permanently in replay mode and omits all its usage, while a higher-valued replayed turn can be accepted as owned; either case corrupts accounting, and the former can prevent --max-cost from being enforced. Use an actual ownership boundary rather than comparing the complete UUID values.
AGENTS.md reference: sdk/typescript/AGENTS.md:L25-L25
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
UUIDv7 alone would not establish this ordering, but the pinned Codex 0.149.1 producer is more specific: thread IDs and turn/submission IDs both use Uuid::now_v7(), whose uuid 1.20.0 implementation guarantees same-process creation order.
Core's fresh-worker path captures parent history before allocating the child ID, then submits its first input afterward in that process (fork path; no reserved child ID). That path does not produce the proposed lower-valued own turn. This does not establish order for arbitrary imported histories, custom ID factories, or unrelated processes; a supported counterexample would change the assessment. This is source verification, not a live-model test. Approval of e7afffc7 still waits for the failed Windows CI run.
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Re-reviewed f3d17829017ee7f4a67ff2f96a9b3fdcd775a780 against declared #630 base 059e7b9de1a9dfc780330e1f9b8b144edc6b471a. No blocking findings in this attribution delta. Production behavior is unchanged from the prior source review; shared fixtures and installed-collector upgrade coverage preserve the relevant checks. The UUID ordering assumption remains limited to Core's fresh-worker, same-process path.
Full current-head CI passes all 27 jobs, including Windows. Local checks were source/diff/AST verification, not a Bun suite.
This approval covers #631's own changes only; it does not approve #630 or other parent changes.
Retain only replay-attribution changes after the restoration fix. Preserve branch history without including the closed-parent implementations. Keep the pinned fresh-worker UUID ordering qualification and combined installed-cache regression coverage.
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed ccd0f7241abcc05b20a4b6d04a4509d66c675adc targeting main. The seven-file attribution diff is unchanged against current main 6ec373cb502511f221766afb9381465f8f9301e6 (merge base 562db120abacdfb8fe2746f0b5ef2935d632153d); the intervening main change is documentation-only and does not change the tested runtime inputs. No blocking findings in this isolated delta.
Full current-head CI passes all 28 jobs, including Windows. The older same-head runs were cancelled during retargeting and do not override this complete matrix. Local integrated verification passed 390 Bun tests (15 platform skips), 20 Python collector tests, types/build/formatting, and the package check. No local native Windows or live-model/billing run is claimed.
The existing producer qualification remains limited to the pinned Core fresh-worker, same-process path. This approval does not resolve the existing review thread; its participants still need to close that discussion before merge. The closed #623/#630 implementations are not included in this main-based delta.
3 participants
Summary
Exclude replayed parent usage from a child scan's budget when the parent turn and child thread were created within the same millisecond.
Changes
main, preserving the original branch history without force-pushing. With fix(sdk): preserve sealed artifacts after post-scan failure #626's restoration change now merged, this seven-file diff carries only attribution changes and does not carry the closed fix(plugin): preserve committed diff scan integrity #623/fix(scan): confine local diff inputs to the selected target #630 implementations.0.1.80. The SDK bundle is generated fromplugins/codex-security; generated files are not edited or committed separately.0.1.60and0.1.79: replace a stale collector, verify its installed bytes, and execute its usage calculation. Retain the restoration, stale MCP configuration, safety forwarding, credentials, login, and unrelated-state checks.Testing
Risk and rollout
Pricing, public APIs, and CLI options are unchanged. The ownership rule depends on the pinned fresh-worker producer behavior described above, not on UUIDv7 in general. The earlier review thread remains open for its participants; this restack does not resolve another reviewer's thread or reopen either closed parent.
Public disclosure review
Historical commit contact metadata and restricted links in automated comments remain, so the second attestation is unchecked. This update uses synthetic fixtures and a GitHub noreply commit identity; other authors' comments are unchanged.