Skip to content

fix: clear the last polynomial-redos instance in swift-cache - #1549

Merged
thymikee merged 2 commits into
mainfrom
claude/swift-cache-redos-fix-3733ef
Aug 2, 2026
Merged

fix: clear the last polynomial-redos instance in swift-cache#1549
thymikee merged 2 commits into
mainfrom
claude/swift-cache-redos-fix-3733ef

Conversation

@thymikee

@thymikee thymikee commented Aug 2, 2026

Copy link
Copy Markdown
Member

Summary

  • sanitizeCacheName in src/utils/swift-cache.ts used .replaceAll(/^-+|-+$/g, ''), the exact js/polynomial-redos pattern class fix: clear the polynomial-redos class across main #1546 retired everywhere else in the repo — this instance was missed.
  • Replaces it with the same linear-time edge-dash trim used in packages/replay-test/src/internal/session-test-artifacts.ts (trimEdgeDashes), kept local to this file since it has a single call site.
  • Practical exposure is low (cache names are internally derived), so this is parity/hygiene with fix: clear the polynomial-redos class across main #1546, not a live vulnerability fix.

Test plan

  • Added a counterfactual regression test: an interior dash run ('x' + '-'.repeat(100_000) + 'x') completes in <1s and returns the input unchanged. Verified it fails against the retired regex (~3.2s, well over the 1s budget).
  • pnpm typecheck
  • pnpm lint
  • pnpm format:check
  • vitest run src/utils/__tests__/swift-cache.test.ts

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.
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.92 MB 1.92 MB +87 B
JS gzip 615.6 kB 615.6 kB +53 B
npm tarball 733.7 kB 733.7 kB +63 B
npm unpacked 2.57 MB 2.57 MB +87 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.1 ms 27.3 ms +1.2 ms
CLI --help 56.6 ms 57.2 ms +0.6 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/internal/daemon.js +87 B +53 B

@thymikee

thymikee commented Aug 2, 2026

Copy link
Copy Markdown
Member Author

Reviewed exact head 0515c6a3. The linear trim is correct and the adversarial regression is non-vacuous; all exact-head checks are green. One P2 remains: sanitizeCacheName was changed from a private helper to a production export solely so the new test can import it, contrary to docs/agents/testing.md’s test-interface rule. Keep the sanitizer private and exercise the regression through an existing production interface or a genuinely shared production seam; while adjusting the test, also pin the behavior-preserving all-invalid fallback (---swift-helper). Required validation: rerun the focused swift-cache test plus the affected repository gates.

…h 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-'.
@thymikee

thymikee commented Aug 2, 2026

Copy link
Copy Markdown
Member Author

Addressed the P2: reverted sanitizeCacheName back to a private (unexported) helper and rewrote the regression test to exercise it through compileSwiftSourceText, an existing production seam that already calls it internally — no new production export added.

Test changes (src/utils/__tests__/swift-cache.test.ts):

  • Timing pin: drives a cache name with a 100k-char interior dash run through compileSwiftSourceText and asserts the whole call settles in under 1s. (The call itself is expected to reject once it reaches disk I/O, since a name that long exceeds the filesystem's path-component limit — but that happens only after the now-fast sanitize step, so timing the settle either way still proves there's no catastrophic backtracking.)
  • All-invalid fallback pin (new, per review): drives cacheName: '---' through compileSwiftSourceText and asserts the returned executable path's basename starts with swift-helper-, pinning the behavior-preserving --- → swift-helper fallback through the production seam.

Counterfactual evidence:

  • Restored the retired /^-+|-+$/g regex trim locally → the timing pin went red: AssertionError: expected 3428 to be less than 1000.
  • Removed the || 'swift-helper' fallback locally → the fallback pin went red: expected false to be true (basename no longer started with swift-helper-).
  • Both reverted back to the fixed code afterward; full suite is green (6/6) at the pushed commit.

Validation: pnpm typecheck && pnpm lint && pnpm format:check && npx vitest run src/utils/__tests__/swift-cache.test.ts — all green.

Pushed to claude/swift-cache-redos-fix-3733ef.

Generated by Claude Code

@thymikee
thymikee merged commit 4fd0441 into main Aug 2, 2026
30 checks passed
@thymikee
thymikee deleted the claude/swift-cache-redos-fix-3733ef branch August 2, 2026 10:26
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-02 10:26 UTC

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