fix(content-type): replace git-diff with jsdiff to clear Snyk issue - #324
Conversation
git-diff@2.0.7 is unmaintained (last published 2018) and pulls in chalk@2, diff@3, loglevel, shelljs and shelljs.exec. Snyk flagged the only vulnerable path in the monorepo through that tree: Missing Release of Resource after Effective Lifetime [Medium] SNYK-JS-INFLIGHT-6095116 git-diff@2.0.7 > shelljs@0.8.5 > glob@7.2.3 > inflight@1.0.6 Snyk reports no direct upgrade or patch, since git-diff is abandoned. Replace it with diff@^9 (jsdiff): zero runtime dependencies, bundled TypeScript types, dual CJS/ESM. It also removes the dependency on a `git` binary and the temp-file/subprocess round trip git-diff used to shell out through shelljs. buildDiffString now calls createTwoFilesPatch, which emits the `---` and `+++` file headers itself, so the hand-rolled header concatenation is gone. Output was verified equivalent: both patch strings were run through Diff2html.parse and compared on file names, added/deleted line counts, hunk headers and every line type plus content - identical. Identical inputs are also handled better. git-diff returned undefined when both sides matched, which interpolated the literal string "undefined" into the patch; createTwoFilesPatch returns a well-formed patch with no hunks. Drop @types/git-diff, since diff ships its own types. Tests: the existing spec mocked diff2html, so the generated patch was never asserted. Add two cases covering the string handed to Diff2html.parse - one for headers, hunk and changed lines, one for the identical-input case. Both were mutation-checked against a corrupted buildDiffString. Suite: 78 passed, 16 suites, tsc clean, Snyk reports no vulnerable paths across the monorepo. Bump the pinned pnpm-lock.yaml checksum in .talismanrc, which the lockfile change invalidates. The finding is the usual sha512 integrity hashes, not a secret. Note: this commit also carries a pre-existing, uncommitted version bump to 1.5.4 that was already present in package.json. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
There was a problem hiding this comment.
Pull request overview
This PR updates the contentstack-cli-content-type plugin to eliminate the unmaintained git-diff dependency (and its vulnerable transitive tree) by switching unified-patch generation to diff (jsdiff), while keeping the existing compare HTML output flow via diff2html.
Changes:
- Replaced
git-diffwithdiff’screateTwoFilesPatchin the content-type compare output generator. - Strengthened Jest coverage by asserting the unified patch string passed into
Diff2html.parse(including the identical-input case). - Updated dependency/lockfile state, docs references to the new diff implementation, and the
.talismanrclockfile checksum; bumped the package version.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pnpm-lock.yaml |
Removes git-diff and related transitive deps, adds diff, and updates lockfile resolutions. |
packages/contentstack-content-type/src/core/content-type/compare.ts |
Switches diff generation from git-diff to diff.createTwoFilesPatch. |
packages/contentstack-content-type/tests/core/content-type/compare.test.ts |
Adds assertions on the generated patch string provided to diff2html. |
packages/contentstack-content-type/skills/contentstack-cli-content-type/SKILL.md |
Updates documentation to reflect diff usage instead of git-diff. |
packages/contentstack-content-type/skills/code-review/SKILL.md |
Updates dependency review checklist to replace git-diff with diff. |
packages/contentstack-content-type/package.json |
Drops git-diff / @types/git-diff, adds diff, and bumps version to 1.5.4. |
.talismanrc |
Updates the pinned checksum for pnpm-lock.yaml. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
The merge of development into this branch left pnpm-lock.yaml internally inconsistent: an eslint-import-resolver-typescript@3.10.1 snapshot still referenced get-tsconfig@4.14.0 while the packages section had advanced to 4.14.1, and several peer-dependency snapshots (ts-node, @tsconfig/*, jest, eslint resolver permutations) had been dropped. This made `pnpm install --frozen-lockfile` fail with ERR_PNPM_LOCKFILE_MISSING_DEPENDENCY, breaking the run-tests and tsgen-integration jobs at the install step before any test could run. Regenerated with `pnpm install --no-frozen-lockfile` using pnpm 10.28.0, matching the pinned packageManager. Lockfile-only change: no package.json or dependency versions altered. Refreshed the pnpm-lock.yaml checksum in .talismanrc to match, per existing repo practice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/contentstack-content-type/skills/contentstack-cli-content-type/SKILL.md:103
- The docs refer to the diff builder as
diff, which is ambiguous (could be read as the unixdifftool). Clarifying that this is the npmdiff(jsdiff) package will make the dependency swap easier to understand for future maintainers.
- **Compare**: `core/content-type/compare.ts` builds a unified diff from two JSON snapshots (`diff`), parses with **diff2html**, writes a **temporary HTML** file, opens it in the browser (`cli-ux` / `cli.open`). Not a terminal table.
…gan types Addresses Copilot review feedback on #324. contentstack-content-type was the only package of 20 declaring @types/* under dependencies; the other 19 keep them in devDependencies. Type packages are compile-time only and nothing here re-exports their types publicly, so shipping them as runtime deps only inflated the production install. Moved @types/diff2html, @types/table and @types/tmp to devDependencies. The runtime packages they describe (diff2html, table, tmp) stay in dependencies. Dropped @types/hogan.js entirely rather than moving it: hogan is referenced nowhere in this package's src or tests, and hogan.js is not a runtime dependency of any package in the repo, so the types were dead weight. Verified with `tsc -b --force` (clean, emits lib/) and the package suite (16 suites, 78 tests passing). Talisman checksum for pnpm-lock.yaml refreshed via `talisman --checksum`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
Brings in the 27 commits development gained after the previous merge (PR #217 Asset-Scanning-v1, #322, #323 and others), clearing the conflict that was blocking PR #243 and preventing GitHub Actions from producing a merge ref. Only .talismanrc conflicted. Resolved to the checksum for the merged pnpm-lock.yaml (95ad8483..., confirmed via `talisman --checksum`) and moved `version` back to the top level, where talisman expects it — development's side had it indented under the fileignoreconfig entry. Verified before push: - `pnpm install --frozen-lockfile` passes (the auto-merged lockfile is self-consistent; this is the check that caught the broken lockfile in PR #324 and the dropped `open` dependency in the previous merge) - dependency sweep against both merge parents: nothing lost - undeclared-import sweep unchanged from baseline: no new hybrids - test suites for all six packages this merge touched: contentstack-audit 160 passing contentstack-export 485 passing contentstack-external-migrate 42 passing contentstack-import 1736 passing contentstack-query-export 139 passing contentstack-bulk-publish (no test script) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Why
git-diff@2.0.7is unmaintained (last published 2018) and pulls inchalk@2,diff@3,loglevel,shelljsandshelljs.exec. Snyk flagged the only vulnerable path in the entire monorepo through that tree:Snyk lists it under "Issues with no direct upgrade or patch" — because
git-diffis abandoned, there is nothing to bump to. The dependency has to go.What
Replaced it with
diff@^9(jsdiff): zero runtime dependencies, bundled TypeScript types, dual CJS/ESM. As a side effect it also drops the dependency on agitbinary and the temp-file + subprocess round tripgit-diffused to shell out throughshelljs.createTwoFilesPatchemits the---/+++file headers itself, so the hand-rolled header concatenation is gone.@types/git-diffdropped too, sincediffships its own types.Output equivalence
Not assumed — checked. Both the old and new patch strings were run through
Diff2html.parseand compared on file names, added/deleted line counts, hunk headers, and every line's type and content. Identical. The rendered compare view is unchanged.One behaviour actually improves:
git-diffreturnedundefinedwhen both sides matched, which interpolated the literal string"undefined"into the patch.createTwoFilesPatchreturns a well-formed patch with no hunks.Tests
The existing spec mocked
diff2html, so the generated patch was never asserted — the diff-building logic had no real coverage. Added two cases against the string handed toDiff2html.parse:@@hunk, and the actual changed-/+lines"undefined"Both were mutation-checked — corrupting
buildDiffStringmakes them fail, so they catch regressions rather than just passing.Reviewer notes
.talismanrc: the pinnedpnpm-lock.yamlchecksum is bumped. Talisman flags the lockfile'ssha512-...npm integrity hashes as "base64 encoded text" — a false positive, not a secret. The file is already ignored by checksum; the lockfile change invalidated the pin. Same thing 38bebe6 and db3fcc5 did. The pre-commit hook was not bypassed — Talisman and Snyk both ran and passed.1.5.3→1.5.4bump that was already in the working tree, in the samepackage.jsonas the dep change and not cleanly separable. Happy to pull it out if you'd rather it landed on its own.src/core/content-type/compare.ts:18:asyncand nothing awaits it, so a real tmp failure becomes an unhandled rejection and kills the CLI instead of surfacing a readable error. This is the one uncovered branch in the coverage report. Out of scope for a dependency swap — worth a follow-up ticket.🤖 Generated with Claude Code