Skip to content

Auto-delete workspace package-lock.json files#2163

Merged
tjcouch-sil merged 14 commits intomainfrom
dependabot-remove-lock-files
Apr 8, 2026
Merged

Auto-delete workspace package-lock.json files#2163
tjcouch-sil merged 14 commits intomainfrom
dependabot-remove-lock-files

Conversation

@tjcouch-sil
Copy link
Copy Markdown
Member

@tjcouch-sil tjcouch-sil commented Mar 31, 2026

dependabot kept spamming us with silly changes to the workspace package-lock.json files that are unused. But we wanted to keep these files synced with the template so we didn't have merge conflicts with them when updating from templates. Now, instead of having the useless extension package-lock.json files in our repo, this PR removes them and automatically resolves merge conflicts with those files by just deleting the file again. Hopefully we don't have to think about those extension package-lock.json files anymore.

Code Review Summary

Branch: dependabot-remove-lock-files

Base: origin/main

Date: 2026-03-31

Review model: Claude Sonnet 4.6

Files changed: 6

Overview

This branch makes update-from-templates automatically delete package-lock.json files in npm workspace subdirectories (under extensions/) so they don't accumulate and cause merge conflicts during template pulls. The approach adds two new exported functions to git.util.ts: isUnusedWorkspacePackageLock (guards that a given path is a workspace-owned lock file) and resolvePackageLockConflicts (called after a failed subtree pull to auto-resolve any conflicts that are solely these unused lock files). Two helper functions — deleteUnusedPackageLockIfPresent and formatExtensionsRoot — handle proactive deletion during the formatting pass. The branch also includes design documents capturing the spec and implementation plan.

The logic is well-scoped: only files that pass the isUnusedWorkspacePackageLock guard are touched automatically, and workspace patterns are read dynamically from package.json rather than hardcoded. During review, minimatch was moved from dependencies to devDependencies and a clarifying comment was added to the Promise.all conflict-classification block.

API Changes

None. No public API surfaces in lib/platform-bible-react/, lib/platform-bible-utils/, papi.d.ts, or extension type declaration files were modified.

Findings

Critical — Must address before merge

None.

Important — Should address before merge

  • extensions/package.json: minimatch in wrong dependency section (fixed during review: moved from dependencies to devDependencies)

  • extensions/lib/git.util.ts: cwd: repoRoot in the shared region referenced a constant defined outside it (fixed during review: extracted execAsync and repoRoot into a new #region shared with paranext-extension-template block at the top of the file; cachedWorkspaces and getWorkspaces were moved to after the export consts to maintain declaration order. repoRoot is now in the shared region and propagation to the template will include it.)

[Author response: minimatch was a mistake — fixed in-review. For the shared region, an initial attempt to move repoRoot into the existing shared region was reverted by lint (no-use-before-define) because getWorkspaces used it above that region. Fixed properly by creating a new shared region for execAsync/repoRoot at the top of the file and relocating cachedWorkspaces/getWorkspaces below the export consts.]

Minor — Consider

  • git.util.ts ~line 231: Non-deterministic push order in Promise.all conflict classifier (fixed during review: added comment explaining that order doesn't matter since all lock files are removed and remainingConflicts is reporting-only)

  • git.util.ts line.slice(3): No explanation for why rename entries aren't a concern (author removed the explanatory line; not important enough to keep)

  • Commit message typo: 'exensions root' (not worth fixing — branch will be squash-merged)

[Author response: Added clarifying comment for Promise.all ordering. Declined to keep the rename-entry explanation. Typo dismissed — squash merge will clean it up.]

Template Propagation

Shared Regions Modified

  • extensions/lib/git.util.ts:9–16 (new region) — execAsync + repoRoot are now marked as shared with paranext-extension-template. Propagate this new region to the template.
  • extensions/lib/git.util.ts:92– (existing region) — execCommand now uses cwd: repoRoot (previously inline). Propagation already needed for this change; covered by the new shared region above. Note: resolvePackageLockConflicts is paranext-core-specific and should NOT be propagated.

Extension Config Changes

None.

Positive Observations

  • resolvePackageLockConflicts is well-scoped: only auto-resolves files that pass isUnusedWorkspacePackageLock, leaving all other conflict files untouched — minimal blast radius.
  • git status --porcelain v1 (vs v2 used in checkForWorkingChanges) is explicitly documented with a justification comment — good callout of an intentional divergence.
  • Sequential git rm calls are correctly awaited in a for...of loop with an explaining comment, avoiding interleaved git index updates.
  • getWorkspaces caches its result — avoids repeated filesystem reads across the loop.
  • Three-way error logging in handleConflicts (resolved-but-others-remain, not-resolved-with-conflicts, not-a-conflict-error) gives clear diagnostics for each failure mode.
  • isUnusedWorkspacePackageLock has layered defensive checks (filename, directory prefix, workspace membership) before touching anything.
  • Workspace patterns are read dynamically from package.json rather than hardcoded — correct and future-proof.
  • deleteUnusedPackageLockIfPresent correctly swallows only ENOENT and re-throws everything else.

Interview Notes

Author's stated purpose: auto-delete unused workspace package-lock.json files during update-from-templates so merge conflicts with these files don't waste time during template pulls.

minimatch being in dependencies was a mistake — fixed in-review. The shared region repoRoot issue was acknowledged; author plans to propagate manually to paranext-extension-template later. Author understands the tradeoff: fully fixing the shared-region issue in-repo would require moving getWorkspaces into the shared region. No tests added for the new helper functions — author explicitly decided against it for these build tooling scripts.

In-Review Quality Check

Changes made during review:

  • extensions/package.json: minimatch moved to devDependencies
  • extensions/lib/git.util.ts: clarifying comment added to Promise.all block
  • extensions/lib/git.util.ts: new #region shared block created for execAsync/repoRoot; cachedWorkspaces/getWorkspaces moved to after export consts (resolves the shared-region constant issue cleanly)

Results: typecheck passed, lint passed, format passed, tests all passed.

Suggested Review Focus

  • Shared region propagation to paranext-extension-template: The new execAsync/repoRoot shared region (lines 9–16) needs to be propagated to the template, along with the cwd: repoRoot change in execCommand. Author plans to do this as a follow-up — confirm it's tracked.
  • resolvePackageLockConflicts placement: This function is paranext-core-specific (depends on workspace structure). Confirm it should not be in the shared region.

The following is AI-generated slop about the PR. I cut out some stuff that was even more of a waste of time to read. You can ignore it if you want:

Changes

  • isUnusedWorkspacePackageLock — checks whether a repo-root-relative path is an unused workspace lock file by matching its parent directory against the workspace patterns in the root package.json
  • resolvePackageLockConflicts — after a failed git subtree pull, scans git status --porcelain for conflict entries, auto-resolves any unused workspace package-lock.json conflicts via git rm, and returns the remaining conflicts (if any)
  • update-from-templates — wraps both the multi-extension and per-extension subtree pulls in conflict-resolution logic; commits automatically when all conflicts are lock files, or surfaces a clean error when real conflicts remain
  • formatExtensionFolder / formatExtensionsRoot — deletes unused lock files during the post-merge formatting pass (handles lock files introduced by clean merges, not just conflicted ones)
  • RefactoringrepoRoot constant, getWorkspaces() cache, deleteUnusedPackageLockIfPresent helper, handleConflicts function extracted to eliminate duplication; explanatory comments added above all ESLint disables

Testing

  • Integration test: ran npm run update-from-templates end-to-end; auto-resolved 1 multi-template conflict (DU), 1 per-extension conflict (UU), and cleaned up 8 more via formatExtensionFolder
  • TypeScript tests pass
  • Lint passes

Open with Devin

This change is Reviewable

@tjcouch-sil tjcouch-sil changed the base branch from ai/main to main March 31, 2026 15:29
@tjcouch-sil tjcouch-sil changed the title Auto-resolve package-lock.json merge conflicts in update-from-templates Auto-delete package-lock.json during merge conflicts and during formatting in update-from-templates Mar 31, 2026
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 9 additional findings.

Open in Devin Review

@tjcouch-sil tjcouch-sil changed the title Auto-delete package-lock.json during merge conflicts and during formatting in update-from-templates Auto-delete workspace package-lock.json files Mar 31, 2026
@Sebastian-ubs
Copy link
Copy Markdown
Contributor

Is this supposed to only delete the package-lock file during update-from-templates or in general?

If in general this is a significant change:

  • with the lock file some can run npm ci to reliably get the same version as the build does
  • without devs and build have to run npm i which may pull updated minor-minor or minor versions based on what is configured in package.json for each dependency.

So what would be lost without having package lock at all is reliably reproducible builds. Two subsequent builds without repo code change may result in a different result. This is just what needs to be clear and accepted as a risk.

@tjcouch-sil
Copy link
Copy Markdown
Member Author

This PR automatically deletes package-lock.json files in workspaces. These package-lock.json files are not used; rather, all the version information is captured in the package-lock.json of the package.json that lists the workspaces.

Copy link
Copy Markdown
Member

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

This looks like you're applying an update from the multi extension template to paranext-core, but it also looks like you asked superpowers to do it instead of running npm run update-from-templates. Is that going to cause weird conflicts later when we do run npm run update-from-templates?

@lyonsil reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: 4 of 7 files reviewed, 1 unresolved discussion (waiting on tjcouch-sil).


docs/superpowers/plans/2026-03-30-update-from-templates-package-lock.md line 0 at r1 (raw file):
Do you think we want to commit all the plans and specs that will get generated along the way? My main concern is that the spec+plan files might not reflect what is ultimately checked in, leading to immediately out of date/incorrect specs. But maybe there is value in keeping them around? What do you think?

Copy link
Copy Markdown
Member Author

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Yeah, basically I wrote the code here first, then I propagated it over to the templates. I plan to merge this so it's a squash and merge, then I'll update from templates and handle the conflicts after this merges if that's ok.

@tjcouch-sil made 2 comments.
Reviewable status: 2 of 7 files reviewed, 1 unresolved discussion (waiting on lyonsil).


docs/superpowers/plans/2026-03-30-update-from-templates-package-lock.md line at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Do you think we want to commit all the plans and specs that will get generated along the way? My main concern is that the spec+plan files might not reflect what is ultimately checked in, leading to immediately out of date/incorrect specs. But maybe there is value in keeping them around? What do you think?

Nope, I don't think we need these! Whoops; meant to remove them before I announced the PR. Maybe we should ignore these folders. I dunno. Removed.

Copy link
Copy Markdown
Member

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

Ok, makes sense. :lgtm:

@lyonsil reviewed 5 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved.

tjcouch-sil and others added 14 commits April 6, 2026 09:52
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ensionFolder

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lates

- Add repoRoot constant and getWorkspaces() cache to avoid re-reading
  root package.json on every isUnusedWorkspacePackageLock call
- Extract deleteUnusedPackageLockIfPresent helper to deduplicate the
  try/unlink/ENOENT pattern from both formatExtensionsRoot and formatExtensionFolder
- Remove redundant .endsWith('package-lock.json') fast-path from
  resolvePackageLockConflicts (isUnusedWorkspacePackageLock already guards this)
- Extract handleConflicts function to deduplicate the 4-branch merge
  conflict response pattern shared by multi-template and per-extension catch blocks
- Move formatExtensionsRoot into the final Promise.all alongside
  formatExtensionFolder calls — no working changes during subtree work
- Add explanatory comments above all ESLint disable annotations
- Fix stale replaceInFileIgnoreGlobs comment (lock files now deleted proactively)
- Fix lint: eslint-disable for JSON.parse type assertion; replace
  NodeJS.ErrnoException with 'code' in error narrowing pattern

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move minimatch to devDependencies (was incorrectly in dependencies)
- Create new #region shared with paranext-extension-template for execAsync/repoRoot
- Move cachedWorkspaces/getWorkspaces to after export consts for correct declaration order
- Add clarifying comment on non-deterministic Promise.all push order in resolvePackageLockConflicts

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@tjcouch-sil tjcouch-sil force-pushed the dependabot-remove-lock-files branch from b9e4454 to b4bd17a Compare April 6, 2026 14:52
@tjcouch-sil tjcouch-sil merged commit 30c4f4c into main Apr 8, 2026
6 of 7 checks passed
@tjcouch-sil tjcouch-sil deleted the dependabot-remove-lock-files branch April 8, 2026 14:07
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.

3 participants