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. |
Dogfood:
|
| Night | Result |
|---|---|
| 09-24 | workspace-open-large script 354 → 497 and task 469 → 648 |
| 09-02 | workspace-open-small and chat-typing-large-history script and task |
| 08-20 | healthy |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57241b5a54
ℹ️ 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: 8b46b0fba0
ℹ️ 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".
|
@codex security review |
🛡️ Codex Security ReviewSecurity 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: cd597a3b96
ℹ️ 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".
Holding this PR: review budget spent, and the findings are no longer shrinkingState at head
Open threads (left unresolved on purpose):
Prepared fix, not pushed: local commit Decision needed from the owner (either of these):
The tracker PR (issues write) stays local until this PR has merged and a dispatch on main shows the table. A fresh-context readiness check agreed: no code blocker for a read-only summary, but merging needs the owner to decide on the missing Codex approval. Generated with |
Halted: review budget spent, findings not convergingThis PR is halted: it will not be merged in its current form, and no further Codex review will be triggered. Why:
Local fixes that exist but are not pushed: commit 77ebfdc71f uses only the final attempt's summary and warns when a history run has no summaries. Proposed scope reduction (a new local candidate, not pushed and not opened as a PR):
The three open threads stay open. Whether one review round may run on the reduced candidate is up to the owner. Generated with |
cd597a3 to
080abf2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 080abf2982
ℹ️ 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".
| // exits 0: the perf job's own result already marks a failed run red. | ||
| console.error( | ||
| report.problems.length > 0 | ||
| ? `perf report: problems (${report.problems.map((problem) => problem.key).join(", ")})` |
There was a problem hiding this comment.
Sanitize problem keys before writing the job log
When a perf test with a crafted title fails or lacks a summary, test.key is embedded unchanged in problem.key and printed here; a newline followed by a GitHub workflow command such as ::warning::..., or terminal escape bytes, can therefore forge annotations or manipulate the perf-report job log. Sanitize each key before logging it, as the Markdown rendering already does for the same artifact-derived strings.
AGENTS.md reference: AGENTS.md:L154-L154
Useful? React with 👍 / 👎.
Halted after the single review round on the reduced headThe approved one-round exception ran on head 080abf2. The push started the code and security reviews automatically, so no manual trigger was posted. Result:
As agreed for this exception, there is no fix, no push and no further round. The PR is not enqueued. Only the owner can decide the next step. Generated with |
|
Closing without merging. The one declared-final exception review round on the reduced candidate (head The full record lives in #4442: 8 Codex assessments (4 code, 4 security), the validation status, the intended one-line fix, and the preserved branches Generated with |


Summary
Adds a read-only nightly perf report. After the Electron perf job, a new
perf-reportjob writes this run's per-test outcomes and each scenario's metrics to the step summary. It never fails the run, makes no GitHub API calls, and has onlycontents: read.This PR no longer compares runs. History retrieval, baselines and regression classification moved out to later, separately scoped work.
Background
The Perf Profiles nightly was red from 08-01 to 09-24, and nobody noticed. Until now its only output was a Playwright report and raw artifacts, and nothing summarized what each scenario measured. A backtest of earlier nights also showed that
workspace-open-largescript time has been about 40% higher since the 09-21 nightly (commitsf02dd639fd..6f573ea1bc). That is reported only as a finding; another workspace is investigating it.Implementation
src/common/utils/perfReport.tsis the pure logic: validation, Playwright result parsing, attribution, problems and rendering.src/because CI only lints, typechecks and testssrc/**.bun install.scripts/perf/perfReport.tsis a thin CLI that reads the downloaded artifact.playwright.config.tsadds a JSON reporter writingartifacts/perf/playwright-results.json. It is active only whenXUM_E2E_RUN_PERF=1, and that path is already uploaded.perf-profiles.ymladds theperf-reportjob:if: always(), so a failed, timed-out or cancelled perf job is still reported;permissions: contents: read;persist-credentials: false;continue-on-error, so a missing artifact becomes a reported problem.Attribution rules
DevToolsCommandDuration, which task time subtracts;Validation
src/common/utils/perfReport.test.ts, covering attribution, the unavailable cases, filtered runs, run-level problems and hostile input.35994317155: healthy, 12 tests, 12 rows.summary-missingproblem and a flaky warning.results:missingproblem and no attributed rows.artifact:missingproblem.node_modules, runs.make static-checkpasses. actionlint 1.7.7 and zizmor 1.20.0 (online,--min-confidence high) report no findings.Risks
Low. The perf job is unchanged apart from the extra reporter file on perf runs. The new job reads only the same-run artifact and writes the step summary.
perf-workspace-scalestays out of this nightly; see #4436.History of this PR: first version, three review rounds, halt, and scope reduction
57241b5,8b46b0f,cd597a3) also fetched 16 earlier scheduled runs and classified regressions against a rolling baseline.The original description follows.
Summary
Adds a read-only nightly perf trend report. After the Electron perf job, a new
perf-trendjob compares tonight's per-scenario metrics with earlier scheduled runs and writes a table, test outcomes, problems and warnings to the step summary. It never fails the run and has noissuespermission. A follow-up PR adds the tracking issue.Background
The Perf Profiles nightly was red from 08-01 to 09-24 without anyone noticing. It had no history and only a few hard asserts. Backtesting 77 scheduled runs (07-10 to 09-24) also found two regressions that slipped by:
workspace-open-largef02dd639fd..6f573ea1bc)chat-typing-large-historyThese are findings only; another workspace is investigating them.
Implementation
src/common/utils/perfTrend.tsholds the pure logic: metric registry, validation, retry collapsing, trend evaluation, Playwright result parsing, problems, and summary rendering. It lives undersrc/because CI only lints, typechecks and testssrc/**. It imports only./assert, so the job runs it with plain Bun and nobun install.scripts/perf/perfTrend.tsis a thin CLI. It reads this run's artifact and fetches the 16 newest earlier scheduled runs on main (at most 30 days old) through the Actions API. PR-branch runs never enter the baseline. A missing or expired artifact is a warning; any other API error is a problem.playwright.config.tsadds a JSON reporter, only whenXUM_E2E_RUN_PERF=1, writingartifacts/perf/playwright-results.json. That path is already uploaded.perf-profiles.ymladds theperf-trendjob:if: always(),permissions: actions: read, contents: read, pinned actions,persist-credentials: false, and all values passed throughenv.Threshold policy
A metric is regressed when all of the last 3 runs exceed
max(baseline × (1 + rel), baseline + abs). The baseline is the median of the 14 runs before those 3, and needs at least 5 values. Watch means only tonight exceeds the threshold, and it never alerts.Why this policy (noise and backtest)
Problems (the run is "alerting"):
Warnings: flaky or skipped tests, skipped history runs, and scenarios that vanished.
make perf-workspace-scaleNot added. Its worst case (~63 min) exceeds the job budget, and it has no thresholds, its own JSON format, and noisy medians of 3. It is tracked as a separate job in #4436.
Validation
Replays against real artifacts match an independent backtest script exactly:
workspace-open-largescript 354 → 497 and task 469 → 648 regressedworkspace-open-smallandchat-typing-large-historyscript and task regressedNegative cases: an empty artifact gives the "artifact missing" problem. A corrupt summary gives an "unusable summary" problem without crashing. A bad token fails the step loudly.
No-install run: the CLI ran from a copy with no
node_modules.Lint:
make lint-actionspasses (actionlint 1.7.7; zizmor 1.20.0 online, no findings).Branch dispatch: evidence follows in a comment.
Risks
Low. The new job only reads and writes the step summary, and the perf job is unchanged apart from the extra reporter file. The main risk is noise: the report may show
watchrows often, but they never alert.📋 Implementation Plan
Nightly perf trend report and failure alerting (revised: two PRs)
Summary
The work lands as two PRs, split at the permissions boundary.
perfTrendmodule, the perf-only Playwright JSON reporter, and history retrieval through the Actions API.perf-trendjob withactions: readandcontents: read. On every run it writes a per-scenario trend table to$GITHUB_STEP_SUMMARY, together with tests, problems, warnings and confirmed/watch markers.issuespermission.perf-regressiontracking issue, managed with pinned baselines, strict state and a stale-run guard.perf-trackerjob withissues: write. It runs only on schedule, or on aworkflow_dispatchon main without a profile filter.Gate between A and B: A is merged, and one real
workflow_dispatchon main shows the table. If theworkspace-open-largeregression is still active, the confirmed rows should show it.Status updates:
workspace-open-largesince the 09-21 nightly (f02dd639fd..6f573ea1bc), and chat-typing style recalcs 105 → 187 since 07-31. A separate investigator owns them; they are not investigated here.Evidence (verified 2026-09-24)
Unchanged from the first plan revision; kept in
/tmp/perf-trend-plan/plan-v1.md.test.retry: 1. The summary recordstest.titleandtest.file.metricskinds: counters and durations cover the measured window, and durations are in seconds.TaskDurationincludes DevTools overhead, so the trend usesTaskDuration − DevToolsCommandDuration.gh run download -nextracts to<dir>/perf/....src/**is linted, formatted, typechecked and unit-tested.max(b × (1 + rel), b + abs), withbthe median of the 14 runs before the streak. It finds 3 sustained episodes and alerts on 0 spikes.Threshold policy (shared by A and B)
v[0..2]. All three runs must have data; otherwise the result is "incomplete".v[3..16], which needs at least 5 values; otherwise "no baseline".v[0]is over.wallTimeMsScriptDuration× 1000(TaskDuration − DevToolsCommandDuration)× 1000LayoutCountRecalcStyleCountsampleCountJSHeapUsedSize/ 2^20iterationSummary.medianMsThe highest retry per label wins, and that row is marked "retried".
Problems (the run is "alerting"):
success;Warnings: flaky or skipped tests, skipped history runs, watch rows, scenarios with history but no data tonight.
PR-A: read-only trend report
Files
src/common/utils/perfTrend.ts(new, pure, imports only./assert). Exports:METRICSreadScenario,collapseAttemptsevaluateTrendparsePlaywrightResultscollectProblemsrenderSummary,sanitizeInlinesrc/common/utils/perfTrend.test.ts: full evaluation and backtest-style tests (listed below).scripts/perf/perfTrend.ts: a thin CLI.--current <dir> --perf-result <r>.gh apiandgh run download: 16 runs, 30 days, 4 at a time, into a temp dir that is always cleaned up. Missing or expired artifacts are skipped with a warning; any other API error is a "history fetch failed" problem.playwright.config.ts: adds a JSON reporter writingartifacts/perf/playwright-results.json, only whenresolveXumEnvironmentValue("E2E_RUN_PERF", process.env) === "1"..github/workflows/perf-profiles.yml: adds theperf-trendjob.needs: perf-profilesandif: always().ubuntu-latestwith a 15-minute timeout.actions: readandcontents: read.persist-credentials: false), setup-bun 1.3.5 with no install, download-artifact withcontinue-on-error, then run the CLI withGH_TOKENandPERF_RESULTpassed throughenv.Tests
evaluateTrend: confirmed vs watch; 1- and 2-night spikes never confirm; the absolute floor applies; no baseline below 5 values; a missing streak run is incomplete; the baseline excludes the streak; report-only metrics never confirm; the 30-day age cutoff.readScenarioandcollapseAttempts: the highest retry wins; invalid schema or missing metrics are reported.parsePlaywrightResults: failed, flaky and skipped tests, attempts, the first error line and global errors; malformed input.collectProblems: one case per problem and warning kind.PR-A dogfood
workspace-open-largescript and task confirmed rows, with baselines of about 354 and 469. The result must match/tmp/perf-trend-plan/an/bt2.ts(C3).gh api markdown, then screenshot it.workflow_dispatchon the PR branch:perf-trendruns;Gates: the bun test,
make typecheck,make lint,make fmt-check, prettier on the CLI,make lint-actions(confirm zizmor actually ran),make static-check, then the readiness loop. The loop allows 6 rounds, and ends with one fresh-context recommendation beforegh pr merge --squash --auto --match-head-commit.PR-B: tracker (after A merges)
Tracker v1 (deliberately small)
Identity: the lowest-numbered open or closed issue with the
perf-regressionlabel and the body marker<!-- xum-perf-tracker -->. Later episodes reopen it.State: one hidden line, parsed strictly:
<and>are escaped.Stale guard: if this run's
(run_number, attempt)is lower thanstate.run, stop with no writes.Pinned:
Invalid state on an existing tracker: keep the body, never close automatically, post a visible error comment, and fail the step.
Human close: it carries no special meaning. If a problem continues, the next run reopens the tracker. Intentional rebaselining is a separately designed follow-up issue.
Actions:
gh label create --force);Job:
perf-tracker, withneeds: perf-profilesandif: always() && (schedule || (dispatch && ref == main && perf_profiles == '')).actions: read,contents: read,issues: write.perf-tracker.Tests:
ghwrite adapter against a fakeghrunner, including an older rerun (stale guard) and a partial API failure (for example the edit succeeds but the close fails).Dogfood:
Follow-ups
Residual risks
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high