feat(api): Add obfuscation field to ChatCompletionChunk - #3690
Conversation
| import subprocess | ||
| import tempfile | ||
| import textwrap | ||
| import unittest |
|
The Castiron custom-code report is available in the successful CI run: open the job summary or download the This PR also introduces the automatic comment publisher. GitHub only starts that |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8eb167039c
ℹ️ 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".
|
|
||
| - name: Validate the codegen hash and report custom code | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} |
There was a problem hiding this comment.
Remove the token from the PR-controlled report step
For fork-originated pull requests, this job checks out the contributor's head and then executes their version of scripts/castiron/custom_code_report.py while GH_TOKEN is present in its environment. A contributor can therefore modify that script to exfiltrate the short-lived repository credential or exercise its contents: read permissions; the token must not be exposed to code taken from the untrusted PR checkout.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
| env: | ||
| REPORTER_SHA256: 73ecd6290e9803b0d0a93af4ca4dccdbf8648cbd5c0cce51ea65fce28c7da79f |
There was a problem hiding this comment.
Verify the reporter against a trusted checksum
Because this checksum and the reporter are both loaded from the pull-request head, a contributor can modify custom_code_report.py to fabricate a clean status: ok report and update this value so the verification step still passes. The separate trusted publisher then renders that fabricated artifact as a github-actions[bot] comment, so the custom-code check cannot be trusted to detect deliberate customizations; the expected checksum or executable must come from outside the PR-controlled checkout.
Useful? React with 👍 / 👎.
| pull_request: | ||
| types: [opened, synchronize, reopened, ready_for_review] |
There was a problem hiding this comment.
Rerun the report when the pull request base changes
When a pull request is retargeted to another base branch, GitHub emits an edited activity rather than any of the activity types listed here. The head SHA can remain unchanged while its merge base and generated baseline change, so no replacement report is produced and the existing bot comment continues to describe the old base; include the base-change event and recompute the report.
Useful? React with 👍 / 👎.
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Independent two-pass review found one Medium compatibility bug: the new stream-chunk-only obfuscation field propagates into completion snapshots and final parsed completions. Preserve it on raw chunks but exclude it when creating completion snapshots.
| Present on the moderation chunk when moderated completions are requested. | ||
| """ | ||
|
|
||
| obfuscation: Optional[str] = None |
There was a problem hiding this comment.
[Medium] Keep stream-only obfuscation out of final completion objects. Adding this field makes _convert_initial_chunk_into_snapshot copy obfuscation via chunk.to_dict() into ParsedChatCompletionSnapshot; Pydantic allows the extra field, and final parsing carries it into returned completions and their serialized output. That violates the non-streaming completion contract and exposes padding outside raw chunks. Exclude obfuscation while constructing snapshots and cover both the intermediate snapshot and public final completion.
Castiron-Internal-PR: openai/openai-python-internal#48 Castiron-Source-SHA: 1da516e7483ea031e228db8338990b552edbd3a1 Castiron-Public-Base-SHA: 2598d53
8eb1670 to
36bb83b
Compare
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Independent two-pass re-review confirms the prior chunk-only obfuscation leak is fixed: raw chunks retain their padding, copied snapshots and final completions omit it, and regression tests cover absent, empty, and populated values plus serialization. No unrelated runtime changes or new suppressions; substantive CI/CodeQL pass.
Summary
Introduce an optional obfuscation field on ChatCompletionChunk to support size-normalized streamed chunks. The new field is optional and omitted when stream_options.include_obfuscation is false.
Changes