Repository navigation
Conversation
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. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| relay = streaming and not isinstance(session.ctx.client, TrainClientConfig) | ||
| # Exact training records include metadata that providers omit from SSE. | ||
| # Buffer the native JSON response, then stream it to the harness after commit. | ||
| relay = streaming and not body.get("return_token_ids", False) |
There was a problem hiding this comment.
🟡 Medium interception/server.py:599
A streamed request with return_training_metadata commits without the requested training attribution instead of taking the buffered validation path and returning the retryable missing-message_spans error. Because relay checks only return_token_ids, this request bypasses Client.get_response; buffer whenever either training field is requested.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @verifiers/v1/interception/server.py around line 599:
A streamed request with `return_training_metadata` commits without the requested training attribution instead of taking the buffered validation path and returning the retryable missing-`message_spans` error. Because `relay` checks only `return_token_ids`, this request bypasses `Client.get_response`; buffer whenever either training field is requested.
![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: e9418cf8bc
ℹ️ 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".
| delta_native.get(input_field) or "" | ||
| ) | ||
| if chunk is not None: | ||
| self.accumulator.feed(ChatCompletionChunk.model_construct(**chunk)) |
There was a problem hiding this comment.
When the interception server relays a Chat Completions SSE stream, parse_sse_event() returns nested dictionaries, but Pydantic's model_construct() does not convert those nested dictionaries into Choice and delta models. ChatCompletionAccumulator.feed() immediately accesses choice.delta, so the first ordinary chunk raises an AttributeError, which _collect_stream() turns into a provider failure. Validate the chunk or explicitly construct its nested models before passing it to the accumulator.
Useful? React with 👍 / 👎.
| self.message = accumulate_event( | ||
| event=cast(Any, event), current_snapshot=self.message | ||
| ) |
There was a problem hiding this comment.
For every non-ping Anthropic SSE event, parse_sse_event() supplies a plain dictionary, while the SDK's accumulate_event() expects a typed raw stream event and dispatches through attributes such as event.type. The cast() has no runtime effect, so streamed Anthropic calls fail on their first event instead of producing a trace or response; convert the dictionary to the appropriate Anthropic event model before accumulation.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs Human Approval Macroscope's review found this PR not approvable — This PR replaces the evaluation/training client architecture and changes production streaming, token metadata, trace-prefix reuse, and training attribution across multiple components. Its broad runtime and public-API impact, together with unresolved substantive concerns around streamed event and usage parsing, warrants human review. Needs human approval because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
e9418cf to
fe868ee
Compare
|
Macroscope has since reviewed this pull request. An earlier review was skipped by a cost limit; a review has now completed, so that notice no longer applies. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe868ee9a1
ℹ️ 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".
| @@ -216,6 +268,7 @@ def response_from_wire(completion: dict) -> Response: | |||
| finish_reason=finish, | |||
| # The SDK's lenient model carries provider extensions such as `cost`. | |||
| usage=Usage.from_openai(CompletionUsage.construct(**usage)) if usage else None, | |||
There was a problem hiding this comment.
When a chat completion includes prompt_tokens_details or completion_tokens_details—as cached-token and reasoning-token responses commonly do—CompletionUsage.construct(**usage) leaves those nested values as dictionaries. Usage.from_openai() then accesses .cached_tokens or .reasoning_tokens, raising AttributeError and turning an otherwise valid completion into a malformed-upstream 502. Use normal Pydantic validation or parse these nested dictionaries directly.
Useful? React with 👍 / 👎.
| # Attribution and image tensors belong to the trace, not the harness response. | ||
| raw.pop("training_metadata", None) |
There was a problem hiding this comment.
For a non-stream training request, only training_metadata is removed before response.raw is returned to the harness; prompt_token_ids, choice token_ids, sampling_mask, and routed_experts remain in the JSON response. These are trace-only records, and routed-expert arrays can be very large, so every non-stream turn needlessly serializes and transfers them to the harness, potentially dominating memory and IPC costs. Remove the extracted training-only fields from raw before assigning it to response.raw.
Useful? React with 👍 / 👎.
| if body.get("return_training_metadata") and ( | ||
| response.tokens is None or response.tokens.message_spans is None | ||
| ): | ||
| raise ValueError( | ||
| "the inference endpoint did not return training metadata" | ||
| ) |
There was a problem hiding this comment.
When return_training_metadata is requested, this check accepts any non-null message_spans list without verifying that it has one entry per parsed prompt message. Under an inference-server version mismatch or malformed response, _commit_turn() treats omitted entries as unspanned, assigns their prompt tokens to the assistant scaffold, and silently corrupts per-message/content attribution and later prefix reuse. Validate the span count against the model request before committing the turn.
Useful? React with 👍 / 👎.
fe868ee to
43fe096
Compare
60c20f9 to
99b2ebe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43fe0967c4
ℹ️ 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 body.get("return_token_ids") or body.get("return_training_metadata"): | ||
| body = {**body, "stream": False} | ||
| body.pop("stream_options", None) |
There was a problem hiding this comment.
When a native Chat Completions caller combines stream: true with return_token_ids, these lines force the upstream request to JSON and the interception server later synthesizes SSE through ChatDialect.stream_events(). That serializer rebuilds each choice using only index, delta, and finish_reason, so the retained choice-level token_ids and logprobs never reach the caller, even though non-stream requests preserve them. Include those choice extensions in the synthesized chunk or reject this option combination rather than returning an incomplete stream.
Useful? React with 👍 / 👎.
43fe096 to
d679f47
Compare
99b2ebe to
5d754d2
Compare
d679f47 to
c2f5d65
Compare
5d754d2 to
899cbf2
Compare
c2f5d65 to
001d11d
Compare
899cbf2 to
9ef4500
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
001d11d to
dd83287
Compare
9ef4500 to
c777d36
Compare
c777d36 to
177030d
Compare
dd83287 to
67204c5
Compare
177030d to
2704d26
Compare
67204c5 to
670eee5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 670eee50a9
ℹ️ 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".
| usage=Usage.from_openai(CompletionUsage.construct(**usage)) if usage else None, | ||
| tokens=tokens, |
There was a problem hiding this comment.
When a training endpoint returns the required prompt_token_ids and choice token_ids but omits the optional Chat Completions usage object, this accepts the response while leaving Response.usage unset. Consequently record_call() stores no usage and the trace reports zero input/output tokens, so max_input_tokens, max_output_tokens, and max_total_tokens never stop that rollout despite the exact counts being available here. Preserve the former training-client behavior by falling back to usage computed from these two arrays.
Useful? React with 👍 / 👎.
2704d26 to
d9d71f5
Compare
a78f58b to
1092382
Compare
d9d71f5 to
242fac6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1092382a90
ℹ️ 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 isinstance(payload, str): | ||
| arr = np.load(io.BytesIO(binascii.a2b_base64(payload)), allow_pickle=False) |
There was a problem hiding this comment.
When a training server returns a malformed or incompatible string-form routed_experts payload, this decode can raise only after _commit_turn() has already appended all prompt and assistant nodes (and may have updated the head index). The server then reports a failed turn and calls turn.abandon(), which only clears the preview, leaving a phantom branch that later retries can match and that can corrupt the trace. Decode and validate this optional payload before appending nodes, or roll back the graph mutations if attribution fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1092382. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 385cf7e7f9
ℹ️ 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 len(sampling_mask.counts) != len(completion_ids) or int( | ||
| sampling_mask.counts.sum() | ||
| ) != len(sampling_mask.ids): | ||
| raise ValueError("sampling masks must align with completion token IDs") |
There was a problem hiding this comment.
When a packed sampling mask contains negative counts, this validation can still pass because it checks only the number of rows and the aggregate sum (for example, counts [-1, 3] with two IDs). The invalid CSR data is then stored on the trace and exposed through Branch.sampling_mask, where negative row sizes can corrupt or break downstream training. Reject any count below zero before accepting the mask.
Useful? React with 👍 / 👎.
385cf7e to
37bac61
Compare
242fac6 to
fa72b35
Compare
Training and evaluation share one SDK-backed
Clientand flatClientConfig. The inference server owns prompt rendering; Verifiers consumes exact tokens and optional training attribution from native Chat Completions. This removes the local renderer pool, separate train/eval clients, and token bridge.The graph keeps one token sequence. Message spans and content masks refer directly to the actual expanded prompt IDs, including image tokens. Exact token-prefix matching reuses valid prefixes and forks when re-rendering changes history; there is no second renderer-token history or multimodal projection pass.
Chat records accept packed float32 completion logprobs and CSR sampling masks as well as native arrays. Alignment, nonnegative row-count, and finite-logprob checks protect training samples. Training arrays are retained in the trace and removed from harness responses; ordinary token-ID requests retain their native arrays. Logprobs remain optional for CE-only generation.
Stack: #2674 → #2675 → #2676. The server/trainer companion is prime-rl #3660, with native image features supplied by renderers #193.
Routing arrays are decoded and validated before graph mutation. Synthesized Chat SSE preserves the choice-level token and logprob fields retained for ordinary token-ID requests.
Validation on current main (e71c69c): all 175 unit tests, contributor hooks, and type checks passed. A packed-mask probe accepts zero/positive row counts and rejects negative counts. The rebase retains upstream worker-thread delta decoding, runtime/Compose fixes, and the new CI model, reasoning, judge, and sampling tests.
Note
High Risk
Removes train/eval client split and graph renderer-bridge fields—breaking for importers of removed types—and changes how training tokens and prefix reuse are derived from upstream inference responses.
Overview
Unifies eval and training behind one SDK
Clientand flatClientConfig, deletingEvalClient/TrainClient,resolve_client, and the eval|train config discriminant (including renderer pool, client-side tokenization, and bridge tails).The shared
Client._completepath forwards native chat requests, buffers SSE when needed, validates token/logprob/training-metadata responses, and strips internal training arrays from harness JSON whenreturn_training_metadatais set.Training tokens now come from the inference server, not a local renderer:
response_from_wirebuildsTurnTokensfromprompt_token_ids, spans, packed logprobs/masks, and related fields; the trace graph keeps a single expanded prompt token sequence (no renderer/logical dual track or MM placeholder projection), reuses prefixes by exact token identity, and decodes routed-expert payloads before commit.Call sites (interception server, eval/GEPA/judge configs, Hermes harness) default to
ClientConfig; Hermes always uses intercept transport. Tests/integration fixtures follow the new types and streaming flag.Reviewed by Cursor Bugbot for commit 37bac61. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Unify eval and train inference clients into single
Client/ClientConfigEvalClient/TrainClientand the discriminated union with one concreteClienttransport (client.py) and oneClientConfigmodel (configs/client.py);resolve_client,BaseClientConfig,EvalClientConfig, andTrainClientConfigare removed fromverifiers.v1exports.TurnTokens(types.py) to use inference-server token metadata instead of renderer-produced tokens, and updates graph commit (graph.py) to reuse prefix nodes from server-provided prompt token IDs, with routed-expert attribution from base64.npyor array payloads.Response.tokens, with validation that raisesValueErroron malformed metadata.JudgeConfig,GEPAConfig,EvalConfig, the interception server, and the Hermes harness to useClientConfig; Hermes ACP config now always setsHERMES_INTERCEPT_TRANSPORT.TurnTokensbridge/placeholder fields,MessageNode.renderer_token_ids); code importingEvalClientConfig,TrainClientConfig, orresolve_clientfromverifiers.v1will break.Macroscope summarized 385cf7e.