Skip to content

fix: clear the polynomial-redos class across main - #1546

Merged
thymikee merged 4 commits into
mainfrom
fix/polynomial-redos-sweep
Aug 1, 2026
Merged

fix: clear the polynomial-redos class across main#1546
thymikee merged 4 commits into
mainfrom
fix/polynomial-redos-sweep

Conversation

@thymikee

@thymikee thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member

You flagged that the CodeQL finding on #1536 exists elsewhere already — correct, and this clears the whole class on main in one small PR:

Gates: typecheck, lint, format, check:replay-compat (frozen corpus unchanged), fallow zero at main scope; 45/45 in the touched suites plus a 100k-char adversarial slug test asserting sub-second completion.

Refs #1478

@thymikee @claude
fix: clear the polynomial-redos class across main
8807a9b 
Three sites of the same CodeQL js/polynomial-redos family:

- packages/replay-test session-test-artifacts/-discovery slugs trimmed edge
  dashes with /^-+|-+$/g, which backtracks polynomially on long dash runs
  built from caller-supplied paths (alerts #27/#28). Replaced with a shared
  linear trimEdgeDashes.
- src/replay/target-identity.ts's target-v1 annotation line regex had the
  \s+(.*) ambiguity (the shape flagged as alert #29 on the #1536 copy).
  Anchored the payload group on \S so the split point is unique; the only
  caller matches against trimmed lines, so behavior is unchanged.

Adversarial regression test on the slug path (100k-char dash run,
sub-second); the annotation-regex adversarial case is covered on the #1536
package copy and the frozen replay-compat corpus passes here unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 1, 2026 •
edited
Loading

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.92 MB 1.92 MB -612 B
JS gzip 615.7 kB 615.5 kB -230 B
npm tarball 733.8 kB 733.6 kB -224 B
npm unpacked 2.57 MB 2.57 MB -612 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 21.9 ms 21.9 ms +0.1 ms
CLI --help 45.3 ms 45.1 ms -0.2 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/session.js -614 B -233 B
dist/src/sdk-selectors.js +2 B +3 B

@thymikee

thymikee commented Aug 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Re-reviewed 8807a9b. The production fix is clean: trimEdgeDashes is linear and output-compatible, the target-v1 \S boundary preserves trimmed-input semantics, and exact-head CodeQL plus all checks are green.

One blocking test-validity issue remains: the claimed adversarial test uses dashes only at the edges, which the old /^-+|-+$/g handles in one pass, so it passes before the fix. Use a non-terminal interior run such as x + '-'.repeat(n) + x, choose a size/bound that robustly fails the old implementation while remaining generous for O(n), assert the exact unchanged output, and pin the all-dash caller fallbacks (test, suite, and omitted empty session slug). Then this is ready.

@thymikee thymikee mentioned this pull request Aug 1, 2026
Merged
@thymikee @claude
test: make the redos regression fail against the retired regex form
6aea6b4 
The edge-run input matched the old /^-+|-+$/g in one pass; the quadratic
case is an interior run (each dash restarts a -+$ attempt that fails at the
trailing byte). The slug pipeline collapses runs before trimming, so the
test targets trimEdgeDashes directly and asserts the input comes back
byte-identical.

Co-Authored-By: Claude <noreply@anthropic.com>
@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Fixed in 6aea6b483 — you were right that the edge-run input was a one-pass match for the old form. The test now feeds a 100k-char interior run (x + dashes + x) to trimEdgeDashes directly (the slug pipeline collapses runs before trimming, so only a direct call can carry the adversarial shape), asserts the exact byte-identical output, and bounds at a generous 1s. Counterfactually verified: with trimEdgeDashes reverted to the regex form the same test fails at 3201ms; at head it passes instantly, 13/13 in the file.

Generated by Claude Code

@thymikee thymikee mentioned this pull request Aug 1, 2026
Merged
@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 6aea6b4. The direct interior-run regression is now genuinely non-vacuous: the retired regex takes seconds on this exact shape while trimEdgeDashes returns it byte-identically well under the bound. Two blockers remain before readiness:

  1. Remove or use the now-unused buildReplayTestInvocationId import—both Typecheck (TS6133) and Lint & Format fail on it at this head.
  2. Add the requested all-dash fallback assertions for artifact slug (test), invocation ID (suite), and session-name slug omission.

Please rerun the exact-head gates after those changes.

@thymikee thymikee mentioned this pull request Aug 1, 2026
Merged
@thymikee @claude
fix: drop the import the test rewrite orphaned
8039cdf 
Co-Authored-By: Claude <noreply@anthropic.com>
@thymikee

thymikee commented Aug 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Both blockers addressed at 2ae86d93e: the orphaned import was dropped (Typecheck and Lint & Format green again), and the all-dash fallback assertions are added — buildReplayTestArtifactSlug → 'test' (input /tmp/####, since dots are legal slug characters and an all-dash filename legitimately slugs to its extension), buildReplayTestInvocationId → 'suite', and buildReplayTestSessionName omitting the slug segment entirely with no dangling separator. 14/14 in the file; typecheck/lint/format green at head.

Errata on my own process, in the spirit of this review thread: the first push of this commit carried a red version of the new test (my initial artifact-slug input wasn't actually all-dash after slugging — it returned '.ad') because a semicolon-chained local run let the commit proceed past the failure. Caught on the immediate re-run, fixed, amended — but the failure mode was exactly the one this PR's review has been policing.

Generated by Claude Code

@thymikee @claude
test: pin the all-dash fallback identifiers
2ae86d9 
Artifact slug falls back to 'test', invocation id to 'suite', and a
session-name slug that trims to nothing is omitted without a dangling
separator.

Co-Authored-By: Claude <noreply@anthropic.com>
@thymikee
thymikee force-pushed the fix/polynomial-redos-sweep branch from c1d16b7 to 2ae86d9 Compare August 1, 2026 12:41
@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Re-review at 2ae86d93e is clean. The unused import is gone, all three fallback contracts are pinned, and the direct interior-run regression is genuinely counterfactual against the retired polynomial regex while asserting exact output preservation. The production slug helper and target-v1 regex fixes remain semantics-preserving, CodeQL is green, and all exact-head checks pass. Ready for human merge.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 1, 2026
@thymikee
thymikee merged commit e88b50f into main Aug 1, 2026
28 checks passed
@thymikee
thymikee deleted the fix/polynomial-redos-sweep branch August 1, 2026 13:11
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-01 13:11 UTC

@thymikee thymikee mentioned this pull request Aug 2, 2026
Merged
5 tasks
thymikee added a commit that referenced this pull request Aug 2, 2026
@thymikee
fix: clear the last polynomial-redos instance in swift-cache (#1549)
4fd0441 
* fix: clear the last polynomial-redos instance in swift-cache

sanitizeCacheName used /^-+|-+$/g to trim edge dashes, the same
js/polynomial-redos pattern PR #1546 retired everywhere else. Replace
it with the linear-time trim used there, and add a counterfactual
regression test that fails against the old regex on a long interior
dash run.

* fix: keep sanitizeCacheName private, drive redos/fallback pins through compileSwiftSourceText

Addresses PR #1549 reviewer feedback: sanitizeCacheName was exported
solely so the regression test could import it, which docs/agents/testing.md's
test-interface rule forbids. Reverted the export and rewrote the test to
exercise the sanitizer through compileSwiftSourceText, an existing production
seam that already calls it.

- Timing pin: a cache name with a 100k-char interior dash run still resolves
  in sub-second time (the call may reject once it reaches disk I/O due to the
  OS path-component length limit, but that happens only after the now-fast
  sanitize step, so timing the settle either way still proves no catastrophic
  backtracking).
- Fallback pin: a cache name that sanitizes to nothing (e.g. '---') still
  produces the 'swift-helper' fallback, observed via the returned executable
  path.

Counterfactuals (see PR comment for full output):
- Restoring the retired `/^-+|-+$/g` regex trim made the timing pin fail:
  3428ms >= 1000ms.
- Removing the `|| 'swift-helper'` fallback made the fallback pin fail:
  basename did not start with 'swift-helper-'.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@thymikee