fix(codex): add default instructions for /responses/compact#2422
fix(codex): add default instructions for /responses/compact#2422luispater merged 2 commits intorouter-for-me:devfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request ensures that the 'instructions' field is present in the request body for compact executions by defaulting it to an empty string if it is missing. A new unit test was added to verify this behavior. The reviewer suggested improving the robustness of this check by also handling cases where the field is explicitly set to null, rather than just checking for its existence.
|
Updated the compact request normalization to coerce both missing and explicit Also expanded the regression test to cover both cases and re-ran:
|
xkonjin
left a comment
There was a problem hiding this comment.
Code Review
Simple, targeted fix with a clear regression test. No significant issues.
Correctness
- Setting
instructionsto""(empty string) is the right move to satisfy the upstream 400. Just worth confirming: does the upstream API treat an empty string and a missing field identically, or does it have any behavioral difference wheninstructions: ""is explicitly sent? If there's a meaningful distinction (e.g., it overrides a server-side default), that should be noted.
Test Coverage
- The test covers both
missing instructionsandnull instructions— good. Consider also adding a case whereinstructionsis already a non-empty string, to assert that the executor does not overwrite a caller-provided value. - The test validates the exact compact response payload with a string comparison. If the response shape ever gains additional fields, this will break. Asserting specific fields (
id,object) rather than the full JSON string would be more resilient.
Scope
- This fix is isolated to
executeCompact. Does the sameinstructionsinjection logic apply to the standardExecutepath, or is it already handled there separately? If it is handled elsewhere, a brief comment cross-referencing that would avoid future confusion.
Summary:
Why:
Verification: