Repository navigation
fix: avoid double-counting logprobs in streamed chat completions - #2444
Merged
Merged
Conversation
HAYDEN-OAI
approved these changes
Aug 25, 2026
HAYDEN-OAI
left a comment
HAYDEN-OAI
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the streaming accumulator and event paths. This fixes first-chunk logprob duplication and avoids mutating source chunk arrays while preserving the streaming API contract. LGTM.
HAYDEN-OAI
enabled auto-merge
August 25, 2026 17:11
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. |
HAYDEN-OAI
disabled auto-merge
August 25, 2026 17:29
Merged
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Aug 25, 2026
## Summary GitHub omits `workflow_run.pull_requests` for external-fork runs, and querying the upstream repository's commit-association endpoint returns no pull requests for those fork commits. That leaves the required `Castiron / budget-only change` and `Castiron / custom-code budget` contexts permanently expected even when the candidate workflow succeeds. - Centralize trusted Python pull-request association in `custom_code_report.py` and reuse it from trusted report generation, comment publication, and budget evaluation. - Resolve missing associations from the authenticated source run's `head_repository`, including legitimately renamed forks, then independently re-fetch every candidate PR from `openai/openai-node`. - Apply equivalent validation in both privileged JavaScript publishers: required commit statuses and failure-report comments. - Refresh the candidate workflow's pinned reporter SHA-256 after the trusted reporter change. ## Security model Fork-side associations and PR numbers are discovery hints, never authorization. The trusted paths: 1. Re-fetch the workflow run from the upstream Actions API and verify its repository, immutable candidate SHA, workflow path, completion, and run attempt as applicable. 2. Strictly validate the source repository's owner/name syntax and consistency with authenticated `head_repository` metadata; reject traversal-like components and spoofed identities. 3. Re-fetch each hinted PR from the upstream repository and require an open PR with the exact candidate SHA, the exact source head repository, the intended upstream repository and `main` base ref, and exactly one valid current association. 4. Require the current `main` base SHA wherever budget evaluation or status publication needs freshness; preserve existing stale-run behavior and merge-group validation. The existing trusted `workflow_run`/main-checkout boundary, bare Git object store, candidate-artifact isolation, least-privilege job permissions, merge-queue protections, and exact required status names remain unchanged. No candidate workflow definition, mutable ref, contributor artifact, or fork-supplied PR number is trusted. ## Affected contributor PRs - #2463: live fork workflow run `32877584723` has `pull_requests: []`; the updated resolver correctly finds its upstream PR through `cmun2/openai-node`. - #2444 and #2431: independently reproduced the same fork-only association behavior; both PRs merged while this fix was being prepared. ## Verification - `env PYTHONDONTWRITEBYTECODE=1 python3 -m unittest discover -s scripts/castiron -p 'test_custom_code*.py'` — 60 tests pass, with one pre-existing skip. - `go run github.com/rhysd/actionlint/cmd/actionlint@latest .github/workflows/castiron-custom-code.yml .github/workflows/castiron-custom-code-comment.yml` — both workflows pass actionlint v1.7.12. - `ruff format --check scripts/castiron/custom_code_report.py scripts/castiron/custom_code_budget.py scripts/castiron/test_custom_code_report.py scripts/castiron/test_custom_code_budget.py`. - Ruff lint passes when ignoring only the same pre-existing baseline rule findings; `git diff --check` passes. - Executable publisher and Python regression coverage includes external forks with empty run associations, fork-side lookup, same-repository and renamed-fork PRs, malformed/spoofed repositories, unrelated source heads, ambiguous/duplicate/invalid associations, stale heads/bases/run attempts, exact required contexts, and merge groups.
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.
Changes being requested
#accumulateChatCompletionseeds a new choice with the chunk'slogprobsby reference, so on that same iterationchoice.logprobs === logprobsandchoice.logprobs.content.push(...content)pushes the array into itself. One streamed token arrives as two entries infinalChatCompletion().choices[0].logprobs.contentand in thelogprobs.content.donepayload.The initialise branch then does
choice.logprobs = { ...logprobs }, one level deep, so the snapshot keeps the caller's array and the next chunk'spushappends into the chunk the caller passed in.Current tests miss both: the fixture's first logprobs-bearing chunk is
{"content":[],"refusal":null}, sopush(...[])is a no-op, and nothing streams two chunks that each carry logprobs.Fix: seed
logprobs: null, and copy the arrays when initialising.Additional context & links
main.npx vitest run --config vitest.config.mts tests/lib/ChatCompletionStream.test.ts-> 38 passed.oxlintandoxfmt --checkpass.