Skip to content

test: time the SQL stripper as the fastest of five runs - #3494

Merged
gitar-bot[bot] merged 3 commits into
mainfrom
claude/fix-sql-strip-timing-flake
Oct 12, 2026
Merged

gitar-bot[bot] merged 3 commits into
mainfrom
claude/fix-sql-strip-timing-flake

Conversation

@BigSimmo

@BigSimmo BigSimmo commented Oct 12, 2026 •

Copy link
Copy Markdown
Owner

User description

Summary

Before: the ReDoS check in tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts ("Edge Case 10") timed one call to stripMigrationSql against a 100 ms ceiling. Under coverage on a busy runner that single cold call crossed the line on two unrelated test-only PRs (#3492 at about 102 ms, #3493 at about 104 ms, Unit coverage partition (1)).

After: the test keeps the fastest of five calls against the same 100 ms ceiling. Measured locally under coverage, the cold call takes about 49 ms and warm calls about 25 ms, so the guard keeps a wide margin, while catastrophic backtracking would still slow every call and fail.

How: a five-iteration loop that keeps the minimum duration; the content assertions still check the last result. No test skipped, disabled or quarantined; the ceiling is unchanged.

Areas touched: Delivery and assurance

Verification

  • npx vitest run tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts → Tests 70 passed (70); prettier and eslint clean.

Risk and rollout

  • Risk: low, test only.
  • Rollback: revert this PR.
  • Provider or production effects: None.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DCghuougzFFXRF3dQQEcYr


Generated by Claude Code


CodeAnt-AI Description

Reduce false failures in the SQL performance test

What Changed

  • The large-SQL performance check now uses the fastest of five runs, reducing sensitivity to a slow cold run on a busy CI runner.
  • The 100 ms limit and SQL content checks remain in place, so consistently slow processing still fails the test.

Impact

✅ Fewer CI failures from cold SQL-stripping runs
✅ ReDoS slowdowns still trigger the 100 ms limit

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.


Summary by cubic

Fixes the SQL stripper ReDoS check in tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts so cold runs under coverage on busy CI runners no longer trip the 100 ms ceiling, while catastrophic backtracking still fails the test.

  • Adds a one-time warm-up call, then requires every one of five measured runs to stay under 100 ms instead of timing a single cold call.
  • The ceiling and content assertions are unchanged.

Written for commit 7eae0d2. Summary will update on new commits.

View guided diff Turn on auto-fix

The 100 ms ReDoS ceiling failed on two unrelated PRs (about 102 and 104 ms)
because a single cold run under coverage on a busy runner can cross it.
Catastrophic backtracking slows every run, so the fastest of five keeps the
guard while dropping warm-up and runner noise.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DCghuougzFFXRF3dQQEcYr
@codeant-ai

codeant-ai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

🤖 CodeAnt AI — Review Status

Status Commit Started (UTC) Finished (UTC)
✅ Reviewed your PR 0041536 Oct 12, 2026 · 11:57 12:00

@greptile-apps greptile-apps 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.

BigSimmo has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

@codeant-ai

codeant-ai Bot commented Oct 12, 2026

Copy link
Copy Markdown

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 1e393c87-4c30-41c2-af8d-b9d73111fc60

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 12, 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-10-12T12:00:21.286461Z 0041536 PR opened
ℹ️ 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.

@supabase

supabase Bot commented Oct 12, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@claude

claude Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@codeant-ai codeant-ai Bot added the size:S This PR changes 10-29 lines, ignoring generated files label Oct 12, 2026
@codeant-ai

codeant-ai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

🏁 CodeAnt Quality Gate Results

Commit: 7eae0d2b
Scan Time: 2026-10-12 12:20:02 UTC

✅ Overall Status: PASSED

Quality Gate Details

Quality Gate Status Details
Secrets ✅ PASSED 0 secrets found
Duplicate Code ✅ PASSED 0.0% duplicated
SAST ✅ PASSED No security issues
Bugs ✅ PASSED Rating S: No bugs
IAC ✅ PASSED No IAC issues

View Full Results

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Stabilize the SQL stripper performance test with five timed runs

🧪 Tests 🕐 Less than 10 minutes

Grey Divider

AI Description

• Measure the fastest of five SQL-stripping runs to reduce cold-start and CI timing noise.
• Keep the 100 ms ReDoS threshold and assertions that verify the stripped SQL.
Diagram

graph TD
  A["Large SQL payload"] --> B["Five timed calls"] --> C["Fastest duration"] --> D{"Under 100 ms?"}
  B --> E["Last output checks"]
Loading
High-Level Assessment

The focused test-only change retains the existing threshold while reducing sensitivity to a single cold or interrupted run. Raising the threshold would weaken the guard, so the five-run minimum is the better fit.

Files changed (1) +10 / -3

Tests (1) +10 / -3
challenger-m3-2-sql-and-lighthouse-stress.test.tsUse the fastest of five runs for the SQL-stripping stress check +10/-3

Use the fastest of five runs for the SQL-stripping stress check

• Replaces one timed call with five and compares their minimum duration with the unchanged 100 ms threshold. The existing content assertions check the final stripped result.

tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts

@gitar-bot
gitar-bot Bot enabled auto-merge (squash) October 12, 2026 11:58
gitar-bot[bot]
gitar-bot Bot previously approved these changes Oct 12, 2026

@gitar-bot gitar-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gitar has auto-approved this PR (configure)

@codspeed

codspeed Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Merging this PR will improve performance by 29.52%

⚠️ 1 benchmark spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

ℹ️ 1 benchmark has no flame graph

The flamegraph could not be generated because too much time was spent in runtime internals, making the visualization not useful for performance analysis.

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
✅ 7 untouched benchmarks

Performance Changes

Benchmark BASE HEAD Efficiency
⚡ answer-ranking: rankAnswerEvidence 40 results 32.1 ms 24.8 ms +29.52%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing claude/fix-sql-strip-timing-flake (7eae0d2) with main (2686f37)

Open in CodSpeed

@qodo-code-review

qodo-code-review Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Busy runners still fail the timing test 🔴 High 🐞 Bug ☼ Reliability ⭐ New
Description
The assertion uses Math.max(...durations), so a single measured call above 100 ms fails the test
even if the other four are fast. Under coverage on a contended runner, any one of the five warm
calls can encounter the same transient delay this PR aims to tolerate, giving the test five
opportunities to fail on runner noise.
Code

tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts[413]

+      expect(Math.max(...durations)).toBeLessThan(100); // Every warm run must be very fast
Relevance

●●● Strong

Finding directly conflicts with the PR’s fastest-run intent; historical timing-flake fixes favor
tolerating transient runner noise.

PR-#2256
PR-#3064

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The test records five individual durations and compares their maximum with 100 ms; therefore one
delayed run is sufficient to fail it. The Vitest configuration permits four workers, so this timing
check runs in a suite configured for concurrent work.

tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts[399-413]
vitest.config.mts[11-16]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The worst-of-five timing assertion fails when one warm call is delayed by runner contention, so the test remains vulnerable to the CI flake this change aims to remove.

## Fix Focus Areas
- tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts[399-413]

## Recommended Fix
Keep the warm-up, but use an assertion that tolerates an isolated slow measurement while failing when warm runs are consistently over 100 ms, such as requiring at least four of five runs below the ceiling.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Dismiss ↗ | View ↗



Remediation recommended

No findings for this group


Informational

No findings for this group

Grey Divider


Resolved findings

1. Slow runs no longer fail the guard ✗ Dismissed
Description
The timing loop around five synchronous stripMigrationSql calls stores only the minimum duration,
discarding the other four measurements before comparing against the 100 ms ceiling. A regression or
runtime condition that makes repeated large-payload scans exceed 100 ms can still pass whenever one
invocation finishes below the threshold.
Code

tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts[R402-407]

+      let stripped = "";
+      let duration = Number.POSITIVE_INFINITY;
+      for (let run = 0; run < 5; run++) {
+        const start = performance.now();
+        stripped = stripMigrationSql(largeSql);
+        duration = Math.min(duration, performance.now() - start);
Relevance

●●● Strong

Recent accepted test findings reject vacuous measurements; minimum-only timing can hide repeated
slow executions.

PR-#3488
PR-#3454

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed test invokes stripMigrationSql five times and retains only the minimum duration, so
the threshold is applied exclusively to the fastest call. Because each invocation synchronously
scans and joins the full input, four slow executions can be hidden by one fast execution, allowing
intermittent or repeated slow-run regressions to escape the assertion; this is the fragile test
behavior identified by the testing-review requirement.

tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts[402-407]
tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts[402-413]
tests/helpers/migration-sql.ts[49-61]
scripts/pr-policy.mjs[491-545]
Skill: testing-review

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The ReDoS timing guard records `Math.min(...)` across five synchronous scans, allowing four executions over the 100 ms limit to pass whenever one execution is fast enough and weakening detection of intermittent or runtime-sensitive regressions.

## Fix Focus Areas
- tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts[402-413]

## Recommended Fix
Retain multiple iterations to reduce cold-start noise, but do not take the minimum. Perform one unmeasured warm-up call if the first cold run should be excluded, then measure the required calls and assert an aggregate that cannot hide repeated slow executions—preferably that every measured warm run remains below the 100 ms ceiling, or otherwise a documented statistic such as the maximum or an appropriate percentile—while retaining the existing output assertions.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 9 rules
✅ Cross-repo context — repo relationships
Review mode: Auto: ⚖️ Balanced: CI installation and recovery changes carry real workflow reliability risk.

Grey Divider

Tip of the day
💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit 7eae0d2 ⚖️ Balanced

Results up to commit 0041536 🚀 Fast


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required

No findings for this group


Remediation recommended

No findings for this group


Informational

No findings for this group


Resolved findings

1. Slow runs no longer fail the guard ✗ Dismissed
Description
The timing loop around five synchronous stripMigrationSql calls stores only the minimum duration,
discarding the other four measurements before comparing against the 100 ms ceiling. A regression or
runtime condition that makes repeated large-payload scans exceed 100 ms can still pass whenever one
invocation finishes below the threshold.
Code

tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts[R402-407]

+      let stripped = "";
+      let duration = Number.POSITIVE_INFINITY;
+      for (let run = 0; run < 5; run++) {
+        const start = performance.now();
+        stripped = stripMigrationSql(largeSql);
+        duration = Math.min(duration, performance.now() - start);
Relevance

●●● Strong

Recent accepted test findings reject vacuous measurements; minimum-only timing can hide repeated
slow executions.

PR-#3488
PR-#3454

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed test invokes stripMigrationSql five times and retains only the minimum duration, so
the threshold is applied exclusively to the fastest call. Because each invocation synchronously
scans and joins the full input, four slow executions can be hidden by one fast execution, allowing
intermittent or repeated slow-run regressions to escape the assertion; this is the fragile test
behavior identified by the testing-review requirement.

tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts[402-407]
tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts[402-413]
tests/helpers/migration-sql.ts[49-61]
scripts/pr-policy.mjs[491-545]
Skill: testing-review

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The ReDoS timing guard records `Math.min(...)` across five synchronous scans, allowing four executions over the 100 ms limit to pass whenever one execution is fast enough and weakening detection of intermittent or runtime-sensitive regressions.

## Fix Focus Areas
- tests/challenger-m3-2-sql-and-lighthouse-stress.test.ts[402-413]

## Recommended Fix
Retain multiple iterations to reduce cold-start noise, but do not take the minimum. Perform one unmeasured warm-up call if the first cold run should be excluded, then measure the required calls and assert an aggregate that cannot hide repeated slow executions—preferably that every measured warm run remains below the 100 ms ceiling, or otherwise a documented statistic such as the maximum or an appropriate percentile—while retaining the existing output assertions.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Copy link
Copy Markdown
Owner Author

On the Qodo finding "Slow runs no longer fail the guard": leaving this as is. stripMigrationSql is a pure synchronous function, and the test calls it five times with the same input, so a regex or algorithmic regression costs the same on every call and still fails the 100 ms ceiling through the minimum. What the minimum drops is the variation that does not come from the code (JIT warm-up, coverage instrumentation, a busy runner), which is exactly what failed #3492 and #3493 at 102 and 104 ms. Asserting on the slowest or mean run would bring that noise back.


Generated by Claude Code

@qodo-code-review

Copy link
Copy Markdown
Contributor

Qodo Fixer

✅ Committed (1) · ☑ Fixed (1)

Grey Divider

Commits pushed directly to this PR — no separate fix PR opened.

Process — 1 fixed
  • ☑ Fixed: Slow runs no longer fail the guard

@gitar-bot gitar-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gitar has auto-approved this PR (configure)

@codeant-ai

codeant-ai Bot commented Oct 12, 2026

Copy link
Copy Markdown

CodeAnt PR Risk: Low Risk

  • The PR appears safe to merge; the change adjusts performance-test measurement without altering production behavior.
  • Requiring all five runs to stay below 100 ms may make the test more sensitive to transient CI load.

Assessed commit: d9042329d36f

@gitar-bot

gitar-bot Bot commented Oct 12, 2026

Copy link
Copy Markdown
Contributor
Code Review ✅ Approved

🟢 Low risk · The SQL-stripper stress test only changes timing methodology; production code and interfaces are untouched, limiting impact to CI performance-regression detection.

Changes the ReDoS timing check in the SQL stripper test to measure the fastest of five runs instead of a single cold call, keeping the 100 ms ceiling and content assertions. No issues found.

Auto-approved: No blocking issues found.
Please see Auto-approve Docs for details on setting custom approval criteria.

Review coverage

🤖 Auto-approval Approved

📋 Rules No rules evaluated

🧪 Functional validation Not enabled · Set up

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 7eae0d2

@gitar-bot
gitar-bot Bot merged commit 565acb0 into main Oct 12, 2026
42 of 43 checks passed
@gitar-bot
gitar-bot Bot deleted the claude/fix-sql-strip-timing-flake branch October 12, 2026 12:19
BigSimmo pushed a commit that referenced this pull request Oct 12, 2026
The slowest-of-five bound from #3494 still failed once on the loaded
coverage runner (107 ms against 100 ms). A slow stripper or catastrophic
backtracking slows every run, so the median keeps catching it while one
garbage-collection or scheduler pause no longer fails the build.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DCghuougzFFXRF3dQQEcYr
gitar-bot Bot pushed a commit that referenced this pull request Oct 12, 2026
…3497)

* test(webkit): settle the last three Safari release-matrix failures

- On Call More anchors: poll until the bar and the page agree; the bar
  resolves its words one animation frame after the page changes.
- DSM compare remove: click, then poll the URL with toHaveURL. A soft
  ?ids= navigation fires no load event, and Promise.all hid whether the
  click landed.
- Phone hidden tabs: poll briefly for the row to leave, and report the
  band's top, translate, animations and attributes on failure so a
  WebKit-only failure can be read from the CI log.

No test skipped or loosened.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DCghuougzFFXRF3dQQEcYr

* test: judge the SQL stripper timing on the median of five warm runs

The slowest-of-five bound from #3494 still failed once on the loaded
coverage runner (107 ms against 100 ms). A slow stripper or catastrophic
backtracking slows every run, so the median keeps catching it while one
garbage-collection or scheduler pause no longer fails the build.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DCghuougzFFXRF3dQQEcYr

* test: keep an outlier ceiling beside the median SQL timing bound

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DCghuougzFFXRF3dQQEcYr

---------

Co-authored-by: Claude <noreply@anthropic.com>
gitar-bot Bot pushed a commit that referenced this pull request Oct 12, 2026
WebKit now runs in CI and its genuine failures were fixed test-side in
#3492-#3494 and #3501-#3504. The last three were the same race, a keypress
before React attached the handler; the update names the helper to use and
the one check left (the first main run after #3504).


Claude-Session: https://claude.ai/code/session_01DCghuougzFFXRF3dQQEcYr

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🕐 Less than 10 minutes 🟠 Medium blast radius size:S This PR changes 10-29 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants