Extend jscpd coverage to scripts/ and remove all duplication - #1713
Conversation
Add `scripts` to the main .jscpd.json path so the 0% duplication policy now applies to the tooling scripts too, then eliminate every clone it surfaced by extracting shared helpers: - stream-lines.ts: unify readStream/readLines/readText - walk-files.ts: shared recursive file walker - line-column.ts: shared offset -> line/column - run-build-edge.ts: shared `deno task build:edge` runner - not-found.ts: shared Deno NotFound guard - precommit/git.ts: shared runGit/commandValue/isInsideWorkTree - mutation/child-process.ts: shared denoExitCode + termination-signal helpers - project-root.ts: shared toDisplayPath - test-harness.ts: shared runSuiteWithHarness Remaining clones are removed in place via small local helpers, function-type aliases, branch merges, and higher-order wrappers. Behaviour is unchanged; typecheck, lint, cpd (both configs), and the scripts/stripe-mock tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YPmXvHk8ynjRJn6TQrjyMJ
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis change centralizes reusable script helpers for process execution, signals, streams, paths, filesystem traversal, testing, Git operations, deployment, mutation tooling, and source analysis. Existing scripts are refactored to consume these helpers, with benchmark and scan configuration updates. ChangesScript consolidation
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd7f5996c9
ℹ️ 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".
| // the finally below still restores. | ||
| } | ||
| } | ||
| onTerminationSignals(onSignal); |
There was a problem hiding this comment.
When any mutation run leaves mutate() (success, survivor, baseline failure, or interrupt), the finally block still executes for (const signal of signals), but this change removed that local signals binding in favor of onTerminationSignals. In deno task mutation ... that throws ReferenceError: signals is not defined and masks the actual mutation result, so callers can no longer rely on the mutation gate; unregister through the shared helper or keep the local signal list.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 286ca6b. The finally now calls the shared offTerminationSignals(onSignal), symmetric with the onTerminationSignals(onSignal) registration — the stale signals loop is gone.
Added a regression guard for the root cause too: runner.ts had no importing test, so deno task typecheck never type-checked it (that's how the identical bug in isolation.ts was caught — via its test). The typecheck task now also checks scripts/mutation/** and scripts/precommit/**, which fails with TS2552: Cannot find name 'signals' before the fix and passes after. Verified end-to-end by running the mutation entry point.
Generated by Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/find-unused-src.ts`:
- Around line 158-164: Refactor extractBraceExports to use functional
composition instead of mutating names in an imperative loop: obtain all regex
matches, map each match through splitAliasedNames using the existing `#fp`
utilities, and flatten the results with flatMap. Preserve the current regex and
returned export-name behavior.
In `@scripts/mutation/runner.ts`:
- Line 577: Update the cleanup logic in the mutation runner to call
offTerminationSignals(onSignal) from a finally block, replacing the manual local
SIGINT/SIGTERM listener-removal loop. Keep onTerminationSignals(onSignal) for
registration and ensure cleanup runs on every exit path.
In `@scripts/precommit/merge-warning.ts`:
- Around line 10-15: Eliminate the duplicated empty-command validation between
splitCommand and runStep. Export splitCommand, add a context-label parameter for
its error message, and update runStep to call this shared helper instead of
destructuring and checking command inline; adjust all existing callers
accordingly.
- Line 8: Move the generic command runner exports and implementations out of
merge-warning.ts into a dedicated command.ts or git.ts module, including
runCommand, runInteractiveCommand, CommandResult, and RunCommand. Update
runner.ts and other consumers to import these symbols directly from the new
module, and remove the re-export from merge-warning.ts so it contains only
merge-warning logic.
In `@scripts/test-harness.ts`:
- Around line 181-186: In runSuiteWithHarness, narrow the
Deno.remove(JUNIT_PATH) error handling to ignore only Deno errors with name
"NotFound"; rethrow all other removal failures so stale JUnit files cannot be
silently reused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 387c4be1-a5c5-479b-b3aa-b7b64c2246a5
📒 Files selected for processing (33)
.jscpd.jsonscripts/bench/bunny-crypto-smoke.tsscripts/build-site.tsscripts/compact-test-reporter.tsscripts/deploy-edge-lib.tsscripts/deploy-edge.tsscripts/find-slow-tests.tsscripts/find-unused-src.tsscripts/line-column.tsscripts/line-counts-lib.tsscripts/mutation/child-process.tsscripts/mutation/generate.tsscripts/mutation/isolation-state.tsscripts/mutation/isolation.tsscripts/mutation/runner.tsscripts/not-found.tsscripts/precommit/git.tsscripts/precommit/merge-warning.tsscripts/precommit/push.tsscripts/precommit/runner.tsscripts/profile-cold-boot.tsscripts/project-root.tsscripts/run-build-edge.tsscripts/run-tests.tsscripts/safe-upgrade.tsscripts/stream-lines.tsscripts/stripe-mock.tsscripts/stripe-mock/install.tsscripts/test-durations.tsscripts/test-harness.tsscripts/test-quality-audit.tsscripts/unit-tests-report.tsscripts/walk-files.ts
Address CI (100% coverage on scripts/) and PR review feedback: - mutation/runner.ts: the mutate() teardown still referenced the removed local `signals` array, throwing ReferenceError on every real run. Use the shared offTerminationSignals(onSignal) helper. Guard the class of bug by type-checking scripts/mutation and scripts/precommit in the typecheck task (runner.ts had no importing test, so it was never type-checked) — this fails before the fix (TS2552) and passes after. - stream-lines.ts + project-root.ts: add direct unit tests; the extracted readStream/toDisplayPath moved out of the coverage-excluded reporter, so they now need their own coverage. Drop the unreachable `?? ""` on a split result. - test-harness.ts: narrow the JUnit-remove catch to NotFound via rethrowUnlessNotFound so a real removal failure surfaces. - precommit/git.ts: move runCommand/runInteractiveCommand and the shared splitCommand here (with a context label reused by runStep) and drop the re-export shim from merge-warning.ts; update all importers. - find-unused-src.ts: extractBraceExports uses #fp flatMap instead of an imperative push loop. All gates pass: lint, typecheck, cpd (0 clones), and 100% line/branch coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YPmXvHk8ynjRJn6TQrjyMJ
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deno.json`:
- Line 18: Update the root-level typecheck command to explicitly include
scripts/mutation.ts and scripts/precommit.ts alongside their existing recursive
directory globs, ensuring the executed mutation and precommit entrypoints are
typechecked.
In `@test/scripts/project-root.test.ts`:
- Around line 14-16: Add a regression test in the existing project-root path
display tests for `/base/..cache.ts`, expecting `..cache.ts`. Update
`toDisplayPath` in `scripts/project-root.ts` so escape detection matches only
the exact `..` segment or paths beginning with `../`, not any relative string
starting with `..`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cbb9e3cf-a437-4a3d-ad03-0e9fae1f695b
📒 Files selected for processing (14)
deno.jsonscripts/find-unused-src.tsscripts/mutation/runner.tsscripts/precommit-mutation.tsscripts/precommit/git.tsscripts/precommit/merge-warning.tsscripts/precommit/mutation-step.tsscripts/precommit/runner.tsscripts/stream-lines.tsscripts/test-harness.tstest/scripts/mutation-step.test.tstest/scripts/precommit.test.tstest/scripts/project-root.test.tstest/scripts/stream-lines.test.ts
💤 Files with no reviewable changes (1)
- scripts/precommit/merge-warning.ts
…am test - deno.json: also type-check the `scripts/mutation.ts` and `scripts/precommit.ts` entrypoints (the recursive globs covered only the subdirectories, so the top-level scripts the tasks actually run were still unchecked). - project-root.ts: `toDisplayPath` now treats only an exact `..` or a `..<sep>`-prefixed relative path as escaping the base, so a file whose name merely starts with `..` (e.g. `..cache.ts`) renders correctly; add tests for both new branches. - stream-lines test: build the fixture stream with `ReadableStream.from` so it closes deterministically (removes a manually-controlled stream that could leave a pending read at suite shutdown). Full suite green locally: lint, typecheck, cpd (0 clones), 100% coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YPmXvHk8ynjRJn6TQrjyMJ
Main now runs jscpd over scripts/ (#1713) and ships a profiler whose timing helper shares this benchmark child's token shape. The child's two db proxies fold their shared scaffolding into a proxyMembers helper, and record() becomes a curried two-stage function (start captured in the outer stage), which both reads better and no longer collides. Verified by re-running the benchmark: identical output (~2.0 round trips cold). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DDJfd1kwQ72hjGvATnEJJ7
2 participants
What
Adds
scriptsto thepathlist in.jscpd.jsonso the project's non‑negotiable 0% duplication policy now covers the tooling scripts alongsidesrcande2e-payments. Turning it on surfaced 35 clones; this PR removes every one of them sodeno task cpdpasses green on both configs.How
Most clones collapsed into small, intent‑revealing shared helpers:
scripts/stream-lines.tsreadStream/readLines/readTextstream readersscripts/walk-files.tsscripts/line-column.tsscripts/run-build-edge.tsdeno task build:edgespawns inbuild-site/deploy-edgescripts/not-found.tsDeno.errors.NotFoundre‑throw guardsscripts/precommit/git.tsrunGit/commandValue/ work‑tree check shared bypush+merge-warningscripts/mutation/child-process.tsdenosubprocess spawns and SIGINT/SIGTERM registrationPlus in‑place fixes where a whole module wasn't warranted:
project-root.tsgains a sharedtoDisplayPath;test-harness.tsgainsrunSuiteWithHarness(used byrun-testsandfind-slow-tests).mutation/generate.ts: aMutantFnfunction‑type alias collapses repeated(node, content, exhaustive) => Mutant[]signatures; aliteralValueOfTypehelper and a hoistedclippedTextremove the rest.deploy-edge-lib.ts: aScriptCodeFntype alias de‑duplicates the upload/deploy signatures (call sites stay positional, so tests are untouched).safe-upgrade.ts: the identicalnpm:/jsr:parse branches merge into one.find-unused-src.tsandprofile-cold-boot.ts: local helpers (splitAliasedNames,readSource,importersOf,isNeverImported,profileCaching,recordTiming,timeLoop) fold the repeated blocks together.mutation/isolation.ts:killRuns/cleanRunsnow delegate to awithSelectedRunswrapper with named body helpers.Verification
deno task cpd→ 0 clones on both.jscpd.jsonand.jscpd.test.jsondeno task typecheckanddeno task lintpasstest/scripts/(59 files) andtest/lib/stripe-mock/pass; refactored standalone scripts (find-unused-src,line-counts,unit-tests-report,test:quality-audit) run cleanlyBehaviour is unchanged throughout — this is duplication removal, not a functional change.
🤖 Generated with Claude Code
https://claude.ai/code/session_01YPmXvHk8ynjRJn6TQrjyMJ
Generated by Claude Code
Summary by CodeRabbit