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. |
![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: 14498a973e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70460ba55e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
xum/src/browser/features/Tools/WorkflowRunToolCall.tsx
Lines 593 to 594 in 8ad87c3
Preserve failed evaluation diagnostics in event details
When an evaluation event is failed, it normally has no usage, so returning only this field makes the coalesced row non-expandable. Its label retains reason and code but drops statusCode and defect, causing provider diagnostics such as HTTP 429/500 to disappear from the workflow tool card; include those failure fields in the projected detail instead.
ℹ️ 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".
8ad87c3 to
3212f08
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3212f08bbc
ℹ️ 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".
…fault is read Codex round 4 on #4330: a model selected before the initial `getConfig()` resolved was persisted and then overwritten on screen by that read's older value, leaving the card behind the on-disk default. The control row is now a `<fieldset disabled>` until the read lands (disables every descendant control, so ModelSelector needs no new prop). Test: deferred read → controls disabled → read resolves → enabled with the persisted value. _Generated with `xum` • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh` • Cost: `$441.23`_ <!-- mux-attribution: model=coder:anthropic/claude-fable-5-1 thinking=xhigh costs=441.23 -->
3212f08 to
0089f31
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0089f31077
ℹ️ 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".
Paused for non-convergence (review round 5)Successive Codex rounds keep finding asynchronous ownership defects in the new evaluation-model card (rounds 2–5), most recently a reconnect variant of the initial-load race fixed in round 4. Under the review-loop rules this PR is not ready, and another isolated guard is not the right next step. No merge and no new review cycle are being initiated. Findings from rounds 4–5
Context: the card follows the existing Proposed scope reduction (maintainer decision)
Existing fixes and the open findings are preserved; L5/L6 stay unpublished until this is decided. Generated with |
…fault is read Codex round 4 on #4330: a model selected before the initial `getConfig()` resolved was persisted and then overwritten on screen by that read's older value, leaving the card behind the on-disk default. The control row is now a `<fieldset disabled>` until the read lands (disables every descendant control, so ModelSelector needs no new prop). Test: deferred read → controls disabled → read resolves → enabled with the persisted value. _Generated with `xum` • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh` • Cost: `$441.23`_ <!-- mux-attribution: model=coder:anthropic/claude-fable-5-1 thinking=xhigh costs=441.23 -->
0089f31 to
88d8509
Compare
ThomasK33
changed the title
|
Resumed under an explicitly bounded exception: one assessment of the reduced head Scope reduced as proposed in the pause comment: this PR is now timeline/tool-card rendering only (plus the bare-response-model-id comparison fix). The Settings card, its If this assessment finds another systemic issue, the PR stays paused; no further loop is intended. Generated with |
There was a problem hiding this comment.
💡 Codex Review
Key evaluation placement by input hash
When a workflow reuses an evaluation step ID with a different input hash, this event is folded into stepFirstEventSeq and stepTitle, which are keyed only by stepId, even though run.steps stores separate (stepId, inputHash) records. Both timeline rows consequently receive the first invocation's title and phase; index evaluation events by the full tuple and use that tuple when projecting each record.
ℹ️ 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".
Teach the workflow evaluate() resolver the reserved evaluation-only `typesafe` key that main already ships for auto model routing (no new PROVIDER_DEFINITIONS entry): shared resolveTypeSafeCredentials, a createTypeSafeAi construction branch, and the CLI's run-scoped providers copy keeps evaluation-only entries so a TypeSafe-only configuration reaches evaluate(). Offering `typesafe:jev-latest` in the Settings evaluation card moves with the card to its own PR (#4330 was reduced to timeline rendering). _Generated with `xum` • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh` • Cost: `$511.72`_ <!-- mux-attribution: model=coder:anthropic/claude-fable-5-1 thinking=xhigh costs=511.72 -->
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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. |
…line rendering Adds the Settings → Tasks "Workflow Evaluation" card (`EvaluationModelCard`): a `ModelSelector` restricted to evaluation-eligible models, a Clear action, and a client-side route hint when the chosen model would not use its provider's direct route. It persists through the dedicated `config.updateEvaluationDefaults` endpoint. The Workflows timeline projects `evaluate()` facts onto the step view (selected model, response model id, attempt, usage, cached replay) and shows an `evaluation` / `evaluation · cached` badge with a metadata line; the workflow tool card coalesces a step's `evaluation` events into one row that keeps the started title and shows the latest status. Storybook gains the card (with a pinned phone variant) and an evaluation-steps run. --- _Generated with `xum` • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh` • Cost: `$326.41`_ <!-- mux-attribution: model=coder:anthropic/claude-fable-5-1 thinking=xhigh costs=326.41 -->
…a for unfinished rows Codex round 1 on the Settings card and timeline: the card filters the hidden model list and explicit gateway prefixes like the backend resolver and persists before publishing a selection; the timeline keys cached replay state by (stepId, inputHash) and shows the admitted model/attempt for failed and running evaluation rows. --- _Generated with `xum` • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh` • Cost: `$378.37`_ <!-- mux-attribution: model=coder:anthropic/claude-fable-5-1 thinking=xhigh costs=378.37 -->
…red list gates Codex round 2: instead of re-implementing resolver rules client-side, the card asks the backend (new network-free config.checkEvaluationModel, a public projection of the resolver's typed rejection) about the selected model — covering Codex OAuth, custom-shadowed ids, gateway routes, missing keys and policy — with a stale-response guard, and filters both offered lists with the shared policy and custom-provider predicates. --- _Generated with `xum` • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh` • Cost: `$417.19`_ <!-- mux-attribution: model=coder:anthropic/claude-fable-5-1 thinking=xhigh costs=417.19 -->
…, origin policy gate, no publish without API Codex round 3: the backend check re-runs when route preferences or the enforced policy change (not only on selection/providers changes); the offered lists apply the policy to the canonical origin like the resolver does, so a permitted gateway route cannot qualify a denied origin; and a selection made while no API client exists is reported as unsaved instead of being published. --- _Generated with `xum` • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh` • Cost: `$431.97`_ <!-- mux-attribution: model=coder:anthropic/claude-fable-5-1 thinking=xhigh costs=431.97 -->
…fault is read Codex round 4 on #4330: a model selected before the initial `getConfig()` resolved was persisted and then overwritten on screen by that read's older value, leaving the card behind the on-disk default. The control row is now a `<fieldset disabled>` until the read lands (disables every descendant control, so ModelSelector needs no new prop). Test: deferred read → controls disabled → read resolves → enabled with the persisted value. _Generated with `xum` • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh` • Cost: `$441.23`_ <!-- mux-attribution: model=coder:anthropic/claude-fable-5-1 thinking=xhigh costs=441.23 -->
…dering Scope reduction after five review rounds of card-lifecycle findings: the Settings evaluation-model card, its `config.checkEvaluationModel` ORPC method, TasksSection wiring, story/mocks plumbing and IPC test leave this PR (preserved on thomask33/eval-l4c-card for a cohesive load/save lifecycle redesign). What remains is the evaluate() step rendering: projection, WorkflowTimeline and WorkflowRunToolCall, plus the response-model comparison fix — provider response ids are bare (`gpt-5`, or the requested bare id as fallback) while the admitted model string is prefixed (`openai:gpt-5`), so comparing them verbatim rendered a second "↳ gpt-5" line on every ordinary completion. _Generated with `xum` • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh` • Cost: `$511.72`_ <!-- mux-attribution: model=coder:anthropic/claude-fable-5-1 thinking=xhigh costs=511.72 -->
…data row Codex on the reduced head: the step metadata row that now carries the evaluation attempt and token count used Tailwind's generic `tabular-nums`; the repository's `counter-nums` utility is the maintained contract for incrementing numeric UI. _Generated with `xum` • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh` • Cost: `$511.72`_ <!-- mux-attribution: model=coder:anthropic/claude-fable-5-1 thinking=xhigh costs=511.72 -->
6744057 to
236d937
Compare
Teach the workflow evaluate() resolver the reserved evaluation-only `typesafe` key that main already ships for auto model routing (no new PROVIDER_DEFINITIONS entry): shared resolveTypeSafeCredentials, a createTypeSafeAi construction branch, and the CLI's run-scoped providers copy keeps evaluation-only entries so a TypeSafe-only configuration reaches evaluate(). Offering `typesafe:jev-latest` in the Settings evaluation card moves with the card to its own PR (#4330 was reduced to timeline rendering). _Generated with `xum` • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh` • Cost: `$511.72`_ <!-- mux-attribution: model=coder:anthropic/claude-fable-5-1 thinking=xhigh costs=511.72 -->
This branch has not been deployed
1 participant
Summary
Fourth layer of the workflow
evaluate()primitive (stacked on #4329), reduced to timeline rendering:evaluate()steps in the right-sidebar workflow timeline and the workflow tool card (badge, model/usage metadata, coalesced rows). The Settings evaluation-model card that this PR originally also carried was removed after review round 5 (see Scope below).Background
Plan layers: L2a #4321 → L2b #4322 → L3a #4324 → L3b #4329 (runner + CLI) → L4 (this PR) → L5 TypeSafe provider → L6 example + docs. #4324 deferred the "coalesce
evaluationevents in the tool card" finding to this layer.Implementation
projectWorkflowRun:WorkflowStepView.evaluation= admission selection + attempt, plusresponseModelId/usagefrom the committedEvaluationStepResultandcachedfrom the replay event.WorkflowTimelineshows anevaluation/evaluation · cachedbadge and a metadata line (model,↳ responseModelIdonly when it differs from the bare requested id — providers reportgpt-5while the admitted string isopenai:gpt-5—,attempt nwhen > 1, tokens); evaluate steps have no task, so no Open action.WorkflowRunToolCall.getWorkflowDisplayRowscoalescesevaluationevents perstepId + inputHash(all attempts of one step) into one row: latest status,startedtitle preserved when a terminal event lacks one, merged details.App/Workflows→EvaluationSteps(cached / failed / running steps).Scope
Review rounds 2–5 all found asynchronous-ownership defects in the Settings card (route/policy gating, then load race, reconnect load, superseded saves, stale verdict) — see the pause comment. The card, its
config.checkEvaluationModelORPC method, TasksSection wiring, story plumbing and IPC test were removed from this PR and preserved onthomask33/eval-l4c-cardfor a cohesive load/save lifecycle design (single owner of the displayed value, per-API-identity loads, one in-flight operation) in its own PR, which carries the card's review history and the TypeSafe card additions from L5. Until thenevaluationDefaults.modelis set via the existingconfig.updateEvaluationDefaultsendpoint /xum workflow run --evaluation-model.Validation
WorkflowTimeline.test.tsx(+1): the↳ responseModelIdline appears only when the response id differs from the requested bare id (same id → no line; dated snapshot id → shown).projectWorkflowRun.test.ts(+1): evaluation facts from admission + result + cached event; running attempt exposes admission only; agent steps stayundefined.WorkflowRunToolCall.test.tsx(+1): two attempts + a title-less terminal event collapse into one row labelled with the started title; other steps stay separate.App.workflows.stories.tsx(bun x test-storybook … --verbose);make static-checkgreen; Workflows/Tools suites 170/170.Risks
evaluationrows/steps; agent, patch and nested-workflow rendering is untouched.Generated with
xum• Model:coder:anthropic/claude-fable-5-1• Thinking:xhigh• Cost:$511.72