Skip to content

🤖 ci: add nightly perf report (current run, final-attempt attribution) - #4437

Closed
ThomasK33 wants to merge 1 commit into
mainfrom
perf-trend-alerts
Closed

ThomasK33 wants to merge 1 commit into
mainfrom
perf-trend-alerts

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Summary

Adds a read-only nightly perf report. After the Electron perf job, a new perf-report job 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 only contents: 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-large script time has been about 40% higher since the 09-21 nightly (commits f02dd639fd..6f573ea1bc). That is reported only as a finding; another workspace is investigating it.

Implementation

  • src/common/utils/perfReport.ts is the pure logic: validation, Playwright result parsing, attribution, problems and rendering.
    • It lives under src/ because CI only lints, typechecks and tests src/**.
    • It has no imports, so the job runs it with plain Bun and no bun install.
  • scripts/perf/perfReport.ts is a thin CLI that reads the downloaded artifact.
  • playwright.config.ts adds a JSON reporter writing artifacts/perf/playwright-results.json. It is active only when XUM_E2E_RUN_PERF=1, and that path is already uploaded.
  • perf-profiles.yml adds the perf-report job:
    • if: always(), so a failed, timed-out or cancelled perf job is still reported;
    • permissions: contents: read;
    • pinned actions and persist-credentials: false;
    • the same-run artifact download uses continue-on-error, so a missing artifact becomes a reported problem.

Attribution rules

  • Expected scenarios are the tests this run actually selected, taken from the Playwright results. A filtered manual run expects only what it ran.
  • Final attempt only. A scenario's numbers come from its test's final attempt. If that attempt wrote no usable summary, the row shows unavailable. An earlier attempt's numbers are never used instead.
  • Problems:
    • the perf job did not succeed;
    • there is no artifact;
    • the results file is missing or malformed, in which case nothing is attributed;
    • a test failed;
    • a passed test's final attempt wrote no summary;
    • a final-attempt summary is unusable, including a missing DevToolsCommandDuration, which task time subtracts;
    • no tests were selected;
    • Playwright reported global errors.
  • Warnings: flaky tests, skipped tests, and summaries that belong to no selected test.
  • Untrusted text: anything taken from the artifact (labels, titles, errors) is sanitized and rendered inside code spans.

Validation

  • Unit tests: 18 behavioral tests in src/common/utils/perfReport.test.ts, covering attribution, the unavailable cases, filtered runs, run-level problems and hostile input.
  • Local replays with the CLI:
    • Branch-dispatch artifact 35994317155: healthy, 12 tests, 12 rows.
    • A copy where one test's final retry wrote nothing: that row is unavailable, with a summary-missing problem and a flaky warning.
    • A copy filtered to 2 of the 6 workspace-open profiles: healthy, 8 rows, no warnings.
    • An older artifact without results JSON: a results:missing problem and no attributed rows.
    • An empty directory: an artifact:missing problem.
    • A copy of just the two files, with no node_modules, runs.
  • Static checks: make static-check passes. 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-scale stays out of this nightly; see #4436.

History of this PR: first version, three review rounds, halt, and scope reduction
  • The first version (heads 57241b5, 8b46b0f, cd597a3) also fetched 16 earlier scheduled runs and classified regressions against a rolling baseline.
  • Codex code-review findings went 4 → 2 → 3, and security was clean each time. That exhausted the six-assessment budget, and the PR was halted because the findings were not converging.
  • The owner approved one exception: a single review round for this reduced candidate, which drops history and regression classification.
  • The removed code and the policy backtests are kept on a local branch for later work.

The original description follows.

Summary

Adds a read-only nightly perf trend report. After the Electron perf job, a new perf-trend job 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 no issues permission. 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:

Scenario Metric Before After Since
workspace-open-large script / task ms 354 / 469 ~500 / ~650 (+40%) 09-21 nightly (f02dd639fd..6f573ea1bc)
chat-typing-large-history style recalcs 105 187 (+78%) 07-31

These are findings only; another workspace is investigating them.

Implementation

  • src/common/utils/perfTrend.ts holds the pure logic: metric registry, validation, retry collapsing, trend evaluation, Playwright result parsing, problems, and summary rendering. It lives under src/ because CI only lints, typechecks and tests src/**. It imports only ./assert, so the job runs it with plain Bun and no bun install.
  • scripts/perf/perfTrend.ts is 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.ts adds a JSON reporter, only when XUM_E2E_RUN_PERF=1, writing artifacts/perf/playwright-results.json. That path is already uploaded.
  • perf-profiles.yml adds the perf-trend job: if: always(), permissions: actions: read, contents: read, pinned actions, persist-credentials: false, and all values passed through env.

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.

Metric Rule
Script ms, task ms (TaskDuration − DevTools overhead) +25% and +50 ms
Layouts +25% and +5
Style recalcs +25% and +10
React renders +25% and +5
Hunk step median +50% and +3 ms
Wall ms, heap MB report only (CV up to 30%, bimodal GC)
Why this policy (noise and backtest)
  • Robust CV across 77 runs: wall 5–30%, script and task 4–34%, counts 0–40%.
  • Comparing a single night with the median alerts on 30 of 77 nights.
  • The 3-run rule confirms the 3 sustained step changes in that history and never fires on a one- or two-night spike.
  • Requiring only 2 runs adds 2 spike episodes.

Problems (the run is "alerting"):

  • the perf job did not succeed;
  • the artifact or the results JSON is missing or malformed;
  • a test failed;
  • a passed test wrote no summary;
  • a summary is invalid;
  • there are zero summaries;
  • a history fetch failed;
  • a regression is confirmed.

Warnings: flaky or skipped tests, skipped history runs, and scenarios that vanished.

make perf-workspace-scale

Not 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:

    Night Result
    09-24 workspace-open-large script 354 → 497 and task 469 → 648 regressed
    09-02 workspace-open-small and chat-typing-large-history script and task regressed
    08-20 healthy
  • Negative 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-actions passes (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 watch rows 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.

  • PR-A (read-only).
    • A pure perfTrend module, the perf-only Playwright JSON reporter, and history retrieval through the Actions API.
    • A new perf-trend job with actions: read and contents: read. On every run it writes a per-scenario trend table to $GITHUB_STEP_SUMMARY, together with tests, problems, warnings and confirmed/watch markers.
    • No issues permission.
  • PR-B (tracker). Branched from main after A merges.
    • One perf-regression tracking issue, managed with pinned baselines, strict state and a stale-run guard.
    • A separate perf-tracker job with issues: write. It runs only on schedule, or on a workflow_dispatch on main without a profile filter.

Gate between A and B: A is merged, and one real workflow_dispatch on main shows the table. If the workspace-open-large regression is still active, the confirmed rows should show it.

Status updates:

Evidence (verified 2026-09-24)

Unchanged from the first plan revision; kept in /tmp/perf-trend-plan/plan-v1.md.

  • Suite: 6 spec files and 12 tests. Each test writes its perf dir before its hard asserts; retries add a directory with test.retry: 1. The summary records test.title and test.file.
  • metrics kinds: counters and durations cover the measured window, and durations are in seconds. TaskDuration includes DevTools overhead, so the trend uses TaskDuration − DevToolsCommandDuration.
  • Artifacts: kept 90 days, about 4.5 MB zipped each. gh run download -n extracts to <dir>/perf/....
  • CI coverage: only src/** is linted, formatted, typechecked and unit-tested.
  • Noise (77 runs): robust CV is 5–30% for wall time, 4–34% for script and task time, and 0–40% for counts. Comparing a single night with the median alerts 30 of 77 nights.
  • Confirm rule backtest: 3 consecutive runs over max(b × (1 + rel), b + abs), with b the median of the 14 runs before the streak. It finds 3 sustained episodes and alerts on 0 spikes.
  • Lifecycle backtest: pinned baselines and 3-run recovery never close a tracker because of absorption.

Threshold policy (shared by A and B)

  • Streak: v[0..2]. All three runs must have data; otherwise the result is "incomplete".
  • Baseline: the median of v[3..16], which needs at least 5 values; otherwise "no baseline".
  • History age: at most 30 days.
  • Confirmed: every streak value is over the threshold. Watch: only v[0] is over.
Metric Source Rule
Wall ms wallTimeMs report only
Script ms ScriptDuration × 1000 +25% and +50 ms
Task ms (TaskDuration − DevToolsCommandDuration) × 1000 +25% and +50 ms
Layouts LayoutCount +25% and +5
Style recalcs RecalcStyleCount +25% and +10
React renders react sampleCount +25% and +5
Heap MB JSHeapUsedSize / 2^20 report only
Hunk step median ms iterationSummary.medianMs +50% and +3 ms

The highest retry per label wins, and that row is marked "retried".

Problems (the run is "alerting"):

  • the perf job result is not success;
  • the current artifact is missing;
  • the results JSON is missing or malformed;
  • a test failed after its retries;
  • a test passed without writing a summary;
  • a summary is invalid or incomplete;
  • zero summaries;
  • confirmed regressions;
  • history fetch failed.

Warnings: flaky or skipped tests, skipped history runs, watch rows, scenarios with history but no data tonight.

PR-A: read-only trend report

Files

  1. src/common/utils/perfTrend.ts (new, pure, imports only ./assert). Exports:
    • METRICS
    • readScenario, collapseAttempts
    • evaluateTrend
    • parsePlaywrightResults
    • collectProblems
    • renderSummary, sanitizeInline
  2. src/common/utils/perfTrend.test.ts: full evaluation and backtest-style tests (listed below).
  3. scripts/perf/perfTrend.ts: a thin CLI.
    • Arguments: --current <dir> --perf-result <r>.
    • Reads the current run directory.
    • Fetches history through gh api and gh 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.
    • Writes the summary. The step exits 0 even when the run is alerting, because this job is a report.
    • A crash (an uncaught error) fails the step loudly.
  4. playwright.config.ts: adds a JSON reporter writing artifacts/perf/playwright-results.json, only when resolveXumEnvironmentValue("E2E_RUN_PERF", process.env) === "1".
  5. .github/workflows/perf-profiles.yml: adds the perf-trend job.
    • needs: perf-profiles and if: always().
    • Runs on ubuntu-latest with a 15-minute timeout.
    • Permissions: actions: read and contents: read.
    • Steps: checkout (persist-credentials: false), setup-bun 1.3.5 with no install, download-artifact with continue-on-error, then run the CLI with GH_TOKEN and PERF_RESULT passed through env.

Tests

  1. 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.
  2. readScenario and collapseAttempts: the highest retry wins; invalid schema or missing metrics are reported.
  3. parsePlaywrightResults: failed, flaky and skipped tests, attempts, the first error line and global errors; malformed input.
  4. collectProblems: one case per problem and warning kind.
  5. Replay: a synthetic step change is confirmed on its 3rd night and stays confirmed while the baseline window still holds pre-step values.
  6. Sanitizing: crafted labels and errors produce no raw HTML and no broken table row.

PR-A dogfood

  1. Replay the 09-24 artifact locally with a dry read. Expect exactly the workspace-open-large script and task confirmed rows, with baselines of about 354 and 469. The result must match /tmp/perf-trend-plan/an/bt2.ts (C3).
  2. Replay 09-02 and 08-20.
  3. Negative cases: empty current dir, corrupt summary, bad token.
  4. A no-install run from a temp copy.
  5. Render the summary through gh api markdown, then screenshot it.
  6. workflow_dispatch on the PR branch:
    • perf-trend runs;
    • the token block shows Actions read, Contents read and Metadata read;
    • the summary shows the table and the tests section from the new JSON;
    • capture screenshots and a short recording, and attach them to the PR.
  7. After merge, dispatch on main and confirm the table. This is the gate for B.

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 before gh 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-regression label and the body marker <!-- xum-perf-tracker -->. Later episodes reopen it.

  • State: one hidden line, parsed strictly:

    <!-- xum-perf-tracker-state: {"v":1,"run":{"number":N,"attempt":A},"pinned":{"<label>/<metric>":baseline},"problems":[...]} -->
    

    < and > are escaped.

  • Stale guard: if this run's (run_number, attempt) is lower than state.run, stop with no writes.

  • Pinned:

    • A newly confirmed key is pinned with its rolling baseline.
    • A pinned key clears only after 3 consecutive valid runs below its pinned threshold. Missing data is unknown, not recovery.
  • 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:

    • no tracker and alerting: create it (with gh label create --force);
    • open tracker and alerting: silently edit the body, and comment only when the problem keys change;
    • open tracker, not alerting, and nothing pinned: post a recovery comment and close it;
    • closed tracker and alerting: reopen, comment and edit;
    • otherwise: do nothing.
  • Job: perf-tracker, with needs: perf-profiles and if: always() && (schedule || (dispatch && ref == main && perf_profiles == '')).

    • Permissions: actions: read, contents: read, issues: write.
    • Concurrency group: perf-tracker.
    • No project dependencies are installed.
    • No separate fallback step: a broken reporter fails the job loudly. Missing or failed runs are covered by an external daily heartbeat, and that limitation is documented.
  • Tests:

    • the pure state functions: parse and serialize, recovery, stale runs;
    • the gh write adapter against a fake gh runner, including an older rerun (stale guard) and a partial API failure (for example the edit succeeds but the close fails).
  • Dogfood:

    • on a PR-branch dispatch, the tracker job is skipped;
    • after merge, a live dispatch on main exercises the real write path, and the issue is screenshotted.

Follow-ups

  • workspace-scale: a separate job with its own timeout, repetitions set to 5 and an adapter. Filed as an issue during PR-A.
  • Intentional rebaseline for the tracker: filed as an issue during PR-B.

Residual risks

  • This workflow cannot detect its own absence, such as a disabled schedule, no runner, or a GitHub outage. An external heartbeat covers that case.
  • A runner-wide change moves all scenarios at once and alerts.
  • Because tracker v1 has no rebaseline, an intended regression keeps the tracker open until the follow-up ships.

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-24T13:22:55.389604Z 080abf2 New commits
🔒 Security Review ✅ Completed 2026-09-24T13:21:43.390597Z 080abf2 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@ThomasK33

ThomasK33 commented Sep 24, 2026 •

Copy link
Copy Markdown
Member Author

Dogfood: workflow_dispatch on this branch (run 35994317155)

  • Performance Profiles passed (12/12 tests; 🤖 perf: stop New Workspace creation controls re-rendering per keystroke #4418 has landed), and Perf trend report ran after it in 22 s.
  • Token scopes in the logs: Perf trend report has Actions: read, Contents: read and Metadata: read. Performance Profiles is unchanged, with Contents: read and Metadata: read.
  • The new perf/playwright-results.json was present, so the header shows per-test counts and 12 scenarios, including the first chat-typing-new-workspace data.
  • Verdict: alerting (regression:workspace-open-large/scriptMs, regression:workspace-open-large/taskMs). The 09-21 regression is still active.

Job summaries are hidden from signed-out viewers, so the screenshot below is the same markdown. It was regenerated from this run's artifact with the CLI and rendered with GitHub's markdown API.

Rendered perf trend summary for run 35994317155

Scenario table continued

summary.mp4

Local replays of past nights match an independent backtest script:

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread scripts/perf/perfTrend.ts Outdated
Comment thread src/common/utils/perfTrend.ts Outdated
Comment thread src/common/utils/perfTrend.ts Outdated
Comment thread scripts/perf/perfTrend.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/common/utils/perfTrend.ts Outdated
Comment thread scripts/perf/perfTrend.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 8b46b0fba0

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread scripts/perf/perfTrend.ts Outdated
Comment thread .github/workflows/perf-profiles.yml Outdated
Comment thread scripts/perf/perfTrend.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

Holding this PR: review budget spent, and the findings are no longer shrinking

State at head cd597a3b96:

  • Codex security review: clean on every head.
  • Codex code review: findings went 4 → 2 → 3 across three heads. All earlier findings are fixed and resolved.
  • Budget: six assessments (code and security for each head) are spent, so I am not pushing again. A push would start another review pair automatically.
  • CI: all six unit shards pass. Codex Comments and Required are red only because three threads are still open.

Open threads (left unresolved on purpose):

  1. Final-attempt selection. If the final retry wrote no summary, an earlier attempt's value is used. This is the substantive one: it matters for the follow-up tracker, which pins confirmed regressions.
  2. History artifact with no summaries. It gives no warning.
  3. Filtered manual dispatch. It produces extra "has history but no data" warnings.

Prepared fix, not pushed: local commit 77ebfdc71f fixes 1 and 2 (about 25 lines, with a test). I propose tracking 3 as a follow-up.

Decision needed from the owner (either of these):

  • Allow one more review round for 77ebfdc71f, or
  • Accept this head without Codex approval, with 1 made a hard prerequisite of the tracker PR.

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 xum • Model: anthropic:claude-opus-5-5 • Thinking: high

@ThomasK33

Copy link
Copy Markdown
Member Author

Halted: review budget spent, findings not converging

This PR is halted: it will not be merged in its current form, and no further Codex review will be triggered.

Why:

  • Budget: three code reviews and three security reviews have used up the six-assessment review budget.
  • Not converging: code-review findings went 4, then 2, then 3 across heads 57241b5, 8b46b0f and cd597a3. Each round found new edge cases in the history and baseline logic.

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):

  • Keep:
    • the perf-only Playwright JSON reporter;
    • a read-only contents: read job that writes this run's metrics table and per-test outcomes to the step summary.
  • Attribution rules:
    • Only the final attempt counts. When that attempt wrote no summary, its data shows as "unavailable". An earlier attempt's numbers are never used instead.
    • The expected scenarios are the tests this run actually selected, so filtered manual runs produce no spurious warnings.
  • Move out, for later and separately scoped work: history retrieval, baseline and streak evaluation, and regression classification. That code and the policy backtests are kept on a local branch.

The three open threads stay open. Whether one review round may run on the reduced candidate is up to the owner.


Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

@ThomasK33 ThomasK33 changed the title 🤖 ci: add nightly perf trend report 🤖 ci: add nightly perf report (current run, final-attempt attribution) Sep 24, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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(", ")})`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@ThomasK33

Copy link
Copy Markdown
Member Author

Halted after the single review round on the reduced head

The approved one-round exception ran on head 080abf2. The push started the code and security reviews automatically, so no manual trigger was posted.

Result:

  • Security review: completed at 13:21:43Z with no findings.
  • Code review: completed at 13:22:55Z with one new finding (P2), still open. scripts/perf/perfReport.ts prints the problem keys to the job log without sanitizing them. A crafted test title could inject a GitHub workflow command such as ::warning::, or terminal escape bytes, into the perf-report log.

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 xum • Model: anthropic:claude-opus-5-5 • Thinking: high

@ThomasK33

Copy link
Copy Markdown
Member Author

Closing without merging. The one declared-final exception review round on the reduced candidate (head 080abf2982f) raised a new code-review finding: problem keys are logged unsanitized, which allows workflow-command injection into the job log. The security review was clean. Under the pre-declared stop rule there is no further fix-and-review round, and this repo requires Codex approval to merge. So the reporter is deferred, not shipped.

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 perf-trend-history-wip and perf-tracker. That review history carries forward, and any continuation needs an explicitly recorded exception. Reopening this PR is preferred over a new one.


Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

@ThomasK33 ThomasK33 closed this Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant