Repository navigation
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 Security Review · Automatically triggeredSecurity 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. |
![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: 02cb183a40
ℹ️ 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".
| if transform := SelectAgentsTransformer(route); transform != nil { | ||
| return transform |
There was a problem hiding this comment.
When a supported Agents create/retrieve/update/list command uses default or --format text output, this transformer removes fields such as instructions, tools, and metadata before writeReadableResource runs. That presenter obtains its hidden flag only from SummarizeResource, which does not recognize Agents and now receives the already-reduced object, so it never emits the usual Summary; use --format json for full data. hint. Carry the omission state into presentation or perform this projection through the existing summary-and-hint path so users are told that the displayed result is incomplete.
Useful? React with 👍 / 👎.
| default: | ||
| return readable.StreamEvent{}, false, false, nil |
There was a problem hiding this comment.
When an Agents stream emits a computer_use_call, this fallback leaves the item unprojected, so showReadableStream sends the complete record to WriteValue. The published ComputerUseCallItemResource permits output.image_url to contain a complete JPEG as a base64 data URL, meaning default text output can dump a very large screenshot into the terminal while the other known tool types omit bulky output. Add explicit computer-use handling that retains identifying/status/title fields and summarizes the screenshot while preserving the complete event in explicit JSON/JSONL formats.
AGENTS.md reference: AGENTS.md:L105-L110
Useful? React with 👍 / 👎.
| func (s *agentsStream[T]) Err() error { | ||
| err := errors.Join(s.err, s.failure, s.source.Err()) |
There was a problem hiding this comment.
When --format explore is used on a terminal and an Agents failure occurs before the explorer's 20-record preload boundary, Err exposes the failure while Next can still return trailing records. ExploreJSONStreamWithOutput interprets that non-nil error after preloading as a terminal iterator failure, writes only the preloaded records, and returns, so any later usage, trace, or diagnostic events are dropped even though this wrapper was introduced specifically to preserve records after a terminal turn failure. Keep the observed failure latent for the explorer until EOF, while still retaining it when a local limit or output error actually stops iteration.
AGENTS.md reference: AGENTS.md:L51-L57
Useful? React with 👍 / 👎.
| case "beta.agents": | ||
| object, fields = "agent", "id name model" | ||
| omitted = "object created_at instructions tools reasoning text service_tier multi_agent metadata" |
There was a problem hiding this comment.
Every valid AgentResource contains the required updated_at field, but it is absent from both the retained and omitted field lists here. Since summarizeResourceFields deliberately rejects any unlisted field, normal create/retrieve/update/list results fall back to the complete object instead of the intended ID/name/model summary, exposing full instructions and tool configuration in default text output. Add updated_at to either the retained or explicitly omitted set and cover it in the Agent fixture.
Useful? React with 👍 / 👎.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity 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 Security Review · Automatically triggeredSecurity 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac55ef33ab
ℹ️ 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".
| default: | ||
| return gjson.Result{}, false, nil | ||
| } | ||
| event, _, hidden, err := summarizeAgentsInputImages(ctx, value, content) |
There was a problem hiding this comment.
When a persisted message or function_call_output contains a syntactically valid object with duplicate image_url keys, summarizeAgentsInputImages receives only the first value returned by GJSON. The first data URL is replaced, but subsequent duplicates remain verbatim, so default/text item-list output can still print an arbitrarily large base64 image while reporting that the record was summarized. Detect duplicate semantic keys and sanitize every matching image slot, or otherwise prevent raw encodings from reaching human output; add a malformed-input regression case.
AGENTS.md reference: AGENTS.md:L76-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae88dd1318
ℹ️ 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".
| if hasResidual { | ||
| remaining = gjson.Parse(residual.String()) | ||
| } | ||
| itemDetails := streamResidual(item, map[string]gjson.Result{"content": remaining}, "id", "type", "role", "status") |
There was a problem hiding this comment.
When an agent.session.turn.item.done event contains a valid assistant message whose status is incomplete, this unconditional omission renders the text as if it completed normally. OutputItemStatusResource explicitly defines incomplete as an item that stopped before completing (api_reference/openapi.transformed.yml:80593-80603), and the event kind only indicates that no more item updates follow. Retain non-success statuses, while continuing to suppress routine in_progress/completed values.
Useful? React with 👍 / 👎.
| itemDetails := streamResidual(item, map[string]gjson.Result{"content": remaining}, "id", "type", "role", "status") | ||
| event.Details = streamResidual(value, map[string]gjson.Result{"item": itemDetails}, | ||
| "type", "event_id", "session_id", "turn_id", "output_index") |
There was a problem hiding this comment.
When an assistant message contains duplicate content keys, item.Get("content") projects only the first array, and this replacement is then applied to every occurrence, so later output-text or unfamiliar parts disappear from default/text output. Detect repeated content containers and fall back to the complete event instead of projecting the first value; add a malformed-input regression case alongside the duplicate-image coverage.
AGENTS.md reference: AGENTS.md:L79-L82
Useful? React with 👍 / 👎.
🛡️ Codex Security Review · Automatically triggeredSecurity 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 The follow-up at a3e5e84 retains incomplete assistant status, unfamiliar fields, and repeated content without corrupting cached state. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3e5e8460d
ℹ️ 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".
| if !IsAgentsStream(route) || !value.IsObject() { | ||
| return "" | ||
| } | ||
| switch value.Get("type").String() { |
There was a problem hiding this comment.
When an Agents stream from a configurable endpoint contains valid JSON with duplicate or case-ambiguous type keys, such as {"type":"error","type":"future.event"}, GJSON's first-match lookup lets this switch record a permanent stream failure. The wrapper then exits nonzero even if subsequent events confirm successful root completion. Validate the event type with agentsUniqueFields before classifying lifecycle failures, consistent with the terminal-outcome path.
AGENTS.md reference: AGENTS.md:L76-L82
Useful? React with 👍 / 👎.
🛡️ Codex Security Review · Automatically triggeredSecurity 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 Security Review · Automatically triggeredSecurity 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e70da6e916
ℹ️ 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".
| var err error | ||
| var messageState *AgentsStreamProjector | ||
| switch kind { | ||
| case "agent.session.turn.output_text.delta", "agent.session.turn.output_text.done": |
There was a problem hiding this comment.
When reasoning summaries are enabled, the API emits the four agent.session.turn.reasoning_summary_* event types defined at api_reference/openapi.transformed.yml:86793-87020, but this switch handles only ordinary output-text events. Those known events therefore fall through to WriteValue, making default/text output print every structured delta and then the complete summary snapshot, even though the later reasoning item deliberately omits its summary. Project the reasoning delta/done and part events through the bounded text assembler (while retaining an incomplete status) so large summaries are rendered incrementally and are not duplicated; explicit JSON/JSONL should remain complete.
AGENTS.md reference: AGENTS.md:L105-L110
Useful? React with 👍 / 👎.
|
Codex Review: Didn't find any major issues. More of your lovely PRs 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". |
🛡️ Codex Security Review · Automatically triggeredSecurity 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 This naming-only follow-up merges the reviewed Output parent at cd1449a into Agents at 6bac987. |
|
Codex Review: Didn't find any major issues. Swish! 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". |
🛡️ Codex Security Review · Automatically triggeredSecurity 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. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed the Agents runtime changes against the recorded Output base, including request retry/idempotency policy, creation acknowledgement, root/subagent turn attribution, terminal failures, incremental reasoning, image summaries, and lossless fallbacks. No blocking findings in this prerequisite.
Verified on the exact head: go test ./pkg/custom ./pkg/transformers -run Agents -count=1 and go vet ./... pass; changed Go sources are gofmt-clean. A temporary mutation of terminal-status recognition failed the pending-turn tests as expected, and the restored tests passed. Hosted checks pass.
This approves the dormant runtime scope only: no public Agents commands are activated by this PR. I did not exercise live API calls or native Windows/Linux explorer behavior.
Castiron custom codeEvaluated main: ✅ No new custom-code files detected. 2 mixed files remain; 0 existing customizations changed. Compared 2 existing customizations unchanged
A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 37959129410 --repo openai/openai-cli \
--name castiron-custom-code-37959129410-1 --dir /tmp/castiron-custom-code-37959129410-1
git apply --stat /tmp/castiron-custom-code-37959129410-1/custom-code.patch
cat /tmp/castiron-custom-code-37959129410-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 866d8770c00c331bf3630b05f2a76870110d2896 6bac9879f7fef4d9e76baa0df6a109c21c89c8c3
python3 scripts/castiron/custom_code_report.py report \
--base 866d8770c00c331bf3630b05f2a76870110d2896 \
--head 6bac9879f7fef4d9e76baa0df6a109c21c89c8c3 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-6bac9879f7fe
cat /tmp/castiron-custom-code-6bac9879f7fe/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
This branch has not been deployed
2 participants
This draft prepares Agents stream handling and readable summaries for a later command update. The helpers remain dormant until generated commands activate them.
What changes
updated_atfields.Idempotency-Key.self_hostedcreation at clean EOF when final input was omitted or null.Creation acknowledgement does not report completed agent work. Supplied inputs, including
[], and GET observation retain normal turn-outcome checks. Failures, unfinished work, conflicting identities, transport errors, and cancellation remain errors.Commands
No commands or flags are added. A separate generated command update must activate these helpers before users can invoke Agents operations.
Explicit formats and extraction retain complete results. Retries preserve the same session, key, body, and SDK retry settings. Observation does not reconnect or resubmit input.
Code
pkg/customowns request policy, lifecycle handling, explorer adaptation, help decoration, and diagnostics.pkg/transformersowns projections and outcome tracking.This prerequisite stacks on the Output foundation, head
d5ca5b4. It adds no generated commands, packages, dependencies, or startup changes.Image validation processes percent escapes incrementally and checks cancellation. Request classification uses finalized headers and input without retaining prompt data.
Reasoning events and item snapshots use the existing stream assembler and bounded identity tracking.
Tested
Candidate
846ac7apassed 130 Agents race-test groups, totaling 1,254 entries, on macOS arm64. No tests failed or skipped.The trusted-main custom-code check passed at 21 lines against a 1,000-line limit.
A separate local generated-command preview passed all 118 new public reasoning subtests.
It also passed 21 existing public groups, totaling 295 entries, including assistant, image, lifecycle, cancellation, and closed-pipe checks.
The five changed files match this candidate; the preview retains older shared foundations. Its generated commands remain outside this PR.
Independent process review passed 64 cases, including 32 exact JSONL controls.
Public checks also passed a producer/consumer handshake and a near-32-MiB reasoning delta with its final snapshot.
A separate exact-source audit passed six groups and nine entries. Capacity, cancellation, and malformed-part rollback checks passed.
The focused native comparison passed two commands and two identical synthetic requests, with checked fixture shutdown.
The candidate shows one reasoning summary and retains incomplete and completed status. Both final images are readable and unclipped.
All eight existing-demo artifacts remained byte-identical. A recorder preflight failure and its corrected successful run remain documented separately.
Current-head CI passed all executed jobs; queue signals skipped.
Code review reported no major issues.
Security review reported no findings.
Native Windows and Linux explorer execution remain untested. No live API workflow was exercised.
The separate command update still needs approved source publication, generated checkpoints, and a compatible SDK release.
Demo
This dormant prerequisite adds no public commands, so it has no standalone user-visible demo. Full Agents workflow evidence belongs with later command activation.