Skip to content

test: scale wall-clock timing budgets on loaded runners - #2509

Merged
steipete merged 2 commits into
mainfrom
codex/deflake-timing-budgets
Aug 3, 2026
Merged

test: scale wall-clock timing budgets on loaded runners#2509
steipete merged 2 commits into
mainfrom
codex/deflake-timing-budgets

Conversation

@steipete

Copy link
Copy Markdown
Owner

Problem

CodexModelsPerformanceTests asserts a hard median < .seconds(1.5) wall-clock budget on the workspace-snapshot build. It failed at 1.635s on a machine that was busy running parallel builds — the code was fine, the machine was loaded. KiroStatusProbeTests has the same shape of assertion for process-timeout and teardown behaviour (< .seconds(7), < .seconds(2)), which degrades the same way under contention.

Both are regression guards: they exist to catch an algorithmic blowup in snapshot building, and a hung child process on teardown. Neither is a latency SLO, so neither should fail because a CI shard was sharing a runner.

Change

  • New TestTimingBudget helper: budgets scale 3x when CI or GITHUB_ACTIONS is set.
  • The perf guard now measures best-of-three instead of the median. The fastest of the three runs is the one least interrupted by other work on the machine, which is exactly what a regression guard wants to measure. Base budget raised 1.5s -> 2s to match.
  • Kiro's two duration assertions routed through the same helper.

The guard still bites: a real O(n^2) regression in snapshot building would blow past 2s on the fastest run, not just the median.

Proof

Local, on this machine:

  • swift test --filter CodexModelsPerformanceTests — passes, fastest run 0.475s against the 2s budget.
  • swift test --filter KiroStatusProbe — 55 tests pass in 28s.
  • Scripts/lint.sh — rc=0.

The workspace-snapshot perf guard failed at 1.635s against a hard 1.5s median
budget while the machine was busy with parallel builds, and the Kiro probe's
teardown budgets are load-sensitive the same way. Both are regression guards
against algorithmic blowups and hung processes, not latency SLOs.

Add `TestTimingBudget` so timing budgets scale 3x under CI, and measure the
perf guard as best-of-three instead of the median — the fastest run is the one
least interrupted by other work.
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels Jul 30, 2026
@clawsweeper

clawsweeper Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 2, 2026, 9:48 PM ET / August 3, 2026, 01:48 UTC.

ClawSweeper review

What this changes

The branch adds a shared test timing-budget helper, scales three wall-clock test limits on CI-like runners, and changes the workspace snapshot performance check from median-of-three to fastest-of-three timing.

Merge readiness

⚠️ Ready for maintainer review - 4 items remain

This owner-authored PR is still necessary: current main retains the hard median and teardown wall-clock assertions, while the branch narrowly makes those test-only regression guards resilient to shared-runner contention. No discrete correctness or security defect was found; retain it for ordinary owner merge handling after the remaining macOS test shard settles.

Priority: P3
Reviewed head: 1f1c729e3d1e8365633bce20df5b8e63a3edf937
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused test-only patch with clear rationale and reported focused validation; the remaining decision is the intended strictness of the regression guard.
Proof confidence 🌊 off-meta tidepool Not applicable: This owner-authored, test-only PR is exempt from the external-contributor real-behavior-proof gate; its body nevertheless records focused test and lint results.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This owner-authored, test-only PR is exempt from the external-contributor real-behavior-proof gate; its body nevertheless records focused test and lint results.
Evidence reviewed 5 items Current main still has the flaky shape: Current main measures the middle of three snapshot durations against a fixed 1.5-second wall-clock limit; it has not incorporated the proposed helper or fastest-run criterion.
Patch keeps functional process assertions intact: The branch changes only the elapsed-time limits for Kiro timeout and termination tests; assertions that the marker is absent, the registry unregisters the PID, and the process exits remain in place.
Baseline provenance: The original hard timing assertions entered the current test files in the late-July merged test expansion; the current patch is a focused follow-up by the same area contributor rather than an unrelated rewrite.
Findings None None.
Security None None.

How this fits together

CodexBar’s Swift test suite validates local usage snapshots and CLI subprocess cleanup. The changed assertions observe elapsed time after those operations and determine whether the suite reports a performance or teardown regression.

flowchart LR
    A[Test runner environment] --> B[Timing budget helper]
    C[Workspace snapshot build] --> D[Three elapsed-time samples]
    D --> E[Fastest-run performance guard]
    B --> E
    F[Kiro CLI subprocess tests] --> G[Timeout and teardown guards]
    B --> G
    E --> H[Test result]
    G --> H
Loading

Decision needed

Question Recommendation
Should the test suite intentionally prefer a fastest-of-three measurement and CI-scaled wall-clock ceilings over the existing median-of-three fixed limits for these regression guards? Accept the load-tolerant guard: Merge after the active macOS shard passes, accepting fastest-of-three and the threefold CI allowance as the intended regression-guard policy.

Why: The code is mechanically small and preserves the non-timing correctness assertions, but the preferred balance between flake resistance and sensitivity to sustained performance regressions is an owner-level test-policy choice.

Before merge

  • Resolve merge risk (P1) - On CI-like runners, fastest-of-three permits two interrupted runs to exceed the base performance limit and raises the effective timing ceilings threefold; that is an intentional trade-off that reduces the guard’s sensitivity to sustained-but-not-universal slowdowns.
  • Resolve merge risk (P1) - The branch was last merged with main at f7000630, while current main is 0c5255c1; GitHub reports it mergeable, but the remaining macOS test shard should complete against the current merge result before landing.
  • Complete next step (P2) - No mechanical repair is identified; the owner should decide the intended regression-guard strictness and merge after the active macOS check completes.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope 27 added, 4 removed across 3 test files The PR is contained entirely within test code and adds no runtime or packaging behavior.
Timing guards changed 3 assertions One snapshot guard and two subprocess lifecycle limits adopt the shared CI-aware budget.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Merge the focused helper once the owner accepts the deliberate fastest-run trade-off and the current macOS test shard passes, keeping the existing snapshot-content and subprocess-cleanup assertions unchanged.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Merge the focused helper once the owner accepts the deliberate fastest-run trade-off and the current macOS test shard passes, keeping the existing snapshot-content and subprocess-cleanup assertions unchanged.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: current main uses fixed wall-clock thresholds for the three identified tests, and the PR body supplies a concrete loaded-runner failure observation. This read-only review did not execute the timing-sensitive tests because repository policy prohibits validation that could trigger local credential prompts without explicit request.

Is this the best way to solve the issue?

Yes, with a maintainer policy choice: centralizing the three CI-aware budgets is narrower and more maintainable than individual ad hoc relaxations, while the existing functional assertions still detect incorrect snapshots and incomplete subprocess cleanup.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 0c5255c1838e.

Labels

Label justifications:

  • P3: This is a targeted test-flake and developer-experience improvement with no direct production behavior change.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This owner-authored, test-only PR is exempt from the external-contributor real-behavior-proof gate; its body nevertheless records focused test and lint results.

Evidence

What I checked:

  • Current main still has the flaky shape: Current main measures the middle of three snapshot durations against a fixed 1.5-second wall-clock limit; it has not incorporated the proposed helper or fastest-run criterion. (Tests/CodexBarTests/CodexModelsPerformanceTests.swift:68, 0c5255c1838e)
  • Patch keeps functional process assertions intact: The branch changes only the elapsed-time limits for Kiro timeout and termination tests; assertions that the marker is absent, the registry unregisters the PID, and the process exits remain in place. (Tests/CodexBarTests/KiroStatusProbeTests.swift:149, c9ce41945a84)
  • Baseline provenance: The original hard timing assertions entered the current test files in the late-July merged test expansion; the current patch is a focused follow-up by the same area contributor rather than an unrelated rewrite. (Tests/CodexBarTests/CodexModelsPerformanceTests.swift:68, 77f2ea0d6d78)
  • Branch scope and provenance: The functional patch commit changes exactly three test files: the snapshot test, two Kiro duration checks, and the new shared helper. The subsequent branch commit only merges the then-current main. (Tests/CodexBarTests/TestTimingBudget.swift:8, c9ce41945a84)
  • Submitted focused validation: The PR body records passing focused snapshot and Kiro test runs plus the repository lint script; the supplied media manifest contains only unrelated proof images from a cross-referenced PR.

Likely related people:

  • steipete: Peter Steinberger introduced the current timing assertions through the late-July test expansion and authored the focused follow-up that adjusts their runner behavior. (role: feature owner and recent area contributor; confidence: high; commits: 77f2ea0d6d78, c9ce41945a84; files: Tests/CodexBarTests/CodexModelsPerformanceTests.swift, Tests/CodexBarTests/KiroStatusProbeTests.swift, Tests/CodexBarTests/TestTimingBudget.swift)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Let the in-progress macOS test shard finish against the current merge result.
  • Confirm that the intended fastest-of-three trade-off is acceptable for these non-SLO timing guards.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (3 earlier review cycles)
  • reviewed 2026-07-30T00:36:13.930Z sha c9ce419 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-31T16:24:53.051Z sha c9ce419 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-03T01:14:50.232Z sha 1f1c729 :: needs maintainer review before merge. :: none

@steipete
steipete merged commit fd7857a into main Aug 3, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant