Skip to content

✨ Add cascade version bump tooling (pnpm versions / pnpm versions:sync)#185

Open
taras wants to merge 2 commits intomainfrom
feat/cascade-versions
Open

✨ Add cascade version bump tooling (pnpm versions / pnpm versions:sync)#185
taras wants to merge 2 commits intomainfrom
feat/cascade-versions

Conversation

@taras
Copy link
Copy Markdown
Member

@taras taras commented Mar 10, 2026

Motivation

When a package like @effectionx/test-adapter gets a version bump, its published dependents (like @effectionx/bdd which has "@effectionx/test-adapter": "workspace:*" in dependencies) must also be bumped so they get republished with the updated dependency range. This was previously a manual process and easy to forget — we just hit this exact problem with the bdd/test-adapter relationship (#183).

Approach

Two new commands backed by Effection-based scripts in .internal/:

Command Purpose
pnpm versions Check — verifies all cascade bumps are present, exits non-zero if any are missing
pnpm versions:sync Fix — auto-applies patch bumps to dependents that need them

How it works:

  • Builds a published dependency graph from workspace:* entries in dependencies and peerDependencies (ignores devDependencies since they're stripped at publish time)
  • Compares each package's version against what's published on npm
  • For any package with an unpublished version, walks the dependency graph transitively to find dependents that also need bumping
  • In check mode: reports missing bumps and fails. In sync mode: applies patch bumps automatically

Files added/changed:

  • .internal/lib/dep-graph.ts — shared dependency graph builder
  • .internal/check-versions.ts — main script with check/sync modes
  • package.json — added versions and versions:sync scripts
  • .github/workflows/test.yaml — added cascade check to CI lint job
  • .policies/version-bump.md — documented cascade rule and tooling

Summary by CodeRabbit

  • New Features

    • Added automated CI check to verify cascading version bumps across workspace packages
    • Added pnpm versions and pnpm versions:sync commands to detect and apply cascade bumps
  • Documentation

    • Updated version-bump policy with cascade rules, examples, and guidance for using the new tooling

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 10, 2026

📝 Walkthrough

Walkthrough

Adds cascade version-bump tooling for PNPM workspaces: dependency-graph utilities, a check/sync script to detect/apply transitive version bumps for published dependents, CI step to run the check, npm scripts, and policy documentation updates.

Changes

Cohort / File(s) Summary
CI & npm scripts
.github/workflows/test.yaml, package.json
Adds a CI step running the versions check and two npm scripts: versions (check) and versions:sync (apply bumps).
Dependency graph utility
.internal/lib/dep-graph.ts
New module exporting WorkspacePackage/DepGraph, buildDepGraph() to gather workspace packages and published workspace deps, and getTransitiveDependents() to compute transitive dependents.
Version check & sync script
.internal/check-versions.ts
New script that builds the dep graph, queries npm for published versions, computes cascade dependents, and either reports missing bumps (check) or increments/writes patch bumps to package.json (sync).
Policy docs
.policies/version-bump.md
Adds a "Cascade Rule" section describing required bumps for published dependents, tooling commands, examples, and verification guidance.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant CLI as pnpm versions
    participant CheckVer as check-versions.ts
    participant DepGraph as dep-graph.ts
    participant NPM as npm Registry
    participant FileSystem as filesystem

    User->>CLI: Execute pnpm versions
    CLI->>CheckVer: Invoke check mode
    CheckVer->>DepGraph: buildDepGraph()
    DepGraph->>FileSystem: Read pnpm-workspace.yaml
    DepGraph->>FileSystem: Read package.json files
    DepGraph-->>CheckVer: Return graph with packages & dependents

    loop For each package
        CheckVer->>NPM: npm view to check published version
        NPM-->>CheckVer: Version or not-found
    end

    alt Package not published
        CheckVer->>DepGraph: getTransitiveDependents()
        DepGraph-->>CheckVer: Set of dependent packages
        CheckVer->>CheckVer: Identify required bumps
        CheckVer->>User: Report missing cascade bumps
    end
Loading
sequenceDiagram
    actor User
    participant CLI as pnpm versions:sync
    participant CheckVer as check-versions.ts
    participant DepGraph as dep-graph.ts
    participant NPM as npm Registry
    participant FileSystem as filesystem

    User->>CLI: Execute pnpm versions:sync
    CLI->>CheckVer: Invoke sync mode
    CheckVer->>DepGraph: buildDepGraph()
    DepGraph->>FileSystem: Read workspace and packages
    DepGraph-->>CheckVer: Return dependency graph

    loop For each package
        CheckVer->>NPM: Check if version published
    end

    loop For unpublished packages
        CheckVer->>DepGraph: getTransitiveDependents()
        DepGraph-->>CheckVer: Affected dependents
        CheckVer->>CheckVer: Compute patch bumps
    end

    loop Update each dependent
        CheckVer->>FileSystem: Read package.json
        CheckVer->>CheckVer: Increment version
        CheckVer->>FileSystem: Write updated package.json
    end

    CheckVer->>User: Print summary of bumps applied
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding cascade version bump tooling with two new pnpm commands.
Description check ✅ Passed The description fully addresses both required template sections with clear motivation, detailed approach with commands table, implementation explanation, and file changes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Policy Compliance ✅ Passed PR complies with all applicable policies. Root package.json is private and exempt from Strict Package.json Metadata Policy. Version Bump Policy does not apply as PR only adds internal tooling, CI configuration, and policy documentation—all explicitly excluded from version bump requirements.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/cascade-versions

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.internal/check-versions.ts:
- Around line 101-115: The current grouping uses pkg.publishedWorkspaceDeps
filtered only by pendingRelease, which loses the transitive cause for packages
in requiredBumps; change the trigger detection in the loop over requiredBumps to
walk dependency edges (starting from each dep in pkg.publishedWorkspaceDeps)
back to the nearest package that is in pendingRelease (or is an original root in
requiredBumps) and record that root trigger so the printed triggerStr shows the
actual triggering package and version; update the logic around requiredBumps /
pkg.publishedWorkspaceDeps / pendingRelease (the block building triggers and
triggerStr) to perform this traversal and map each dependency to its ultimate
triggering package before formatting the output.
- Around line 28-39: The loop over graph.packages currently treats any non-zero
exit from the `npm view ${pkg.name}@${pkg.version}` call (npmCheck/result) as
"version not published" and adds pkg.name to pendingRelease; change this to
distinguish three cases: (1) run `npm view ${pkg.name}@${pkg.version}` and if it
returns exitCode 0 with stdout non-empty, do nothing; (2) if version-check
exitCode !== 0, run a plain `npm view ${pkg.name}` to see if the package
exists—if that exists but version missing, add to pendingRelease; (3) if the
package-check itself failed with a transient/network/auth error (non-zero
exitCode or other error), surface a hard failure (throw or exit) instead of
adding to pendingRelease so `versions:sync` cannot rewrite unrelated packages;
mirror the existence/version split logic used in publish-matrix.ts and use the
same helper/command patterns around npmCheck/result and pendingRelease to locate
and update the behavior.

In @.internal/lib/dep-graph.ts:
- Around line 43-56: This code duplicates a pnpm-workspace.yaml parser and omits
the glob guard; instead reuse the existing parser from .internal/sync-tsrefs.ts:
export the parser there (e.g., parsePnpmWorkspace or parseWorkspaceYaml) and
import it into .internal/lib/dep-graph.ts, replacing the manual loop that builds
workspaces from workspaceYaml and the local workspaces array; ensure the shared
parser preserves the glob-character guard and returns the same string[] shape so
dep-graph.ts can use it without additional logic.
- Around line 145-163: The predicate in collectWorkspaceDeps is too broad
(range.startsWith("workspace:")); narrow it to only match the documented
workspace cascade variants (e.g., workspace:*, workspace:^, workspace:~) by
replacing that check with a strict pattern test on range (for example using a
regex like /^workspace:(\*|~|\^)/) while keeping the existing guards
(depsObject, typeof range) and leaving packageNames and result handling intact.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 43868006-1324-4f27-bb45-4e894a9b0d4b

📥 Commits

Reviewing files that changed from the base of the PR and between 56beee6 and 9a09857.

📒 Files selected for processing (5)
  • .github/workflows/test.yaml
  • .internal/check-versions.ts
  • .internal/lib/dep-graph.ts
  • .policies/version-bump.md
  • package.json

Comment on lines +28 to +39
for (const pkg of graph.packages.values()) {
const npmCheck = yield* x(
"npm",
["view", `${pkg.name}@${pkg.version}`, "version"],
{ throwOnError: false },
);
const result = yield* npmCheck;

if (result.exitCode !== 0 || result.stdout.trim() === "") {
pendingRelease.add(pkg.name);
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
python - <<'PY'
import json
import pathlib
import subprocess

root = pathlib.Path(".")
entries = []

workspace_file = root / "pnpm-workspace.yaml"
for line in workspace_file.read_text().splitlines():
    trimmed = line.strip()
    if trimmed.startswith("-"):
        value = trimmed[1:].strip().strip('"').strip("'")
        if value:
            entries.append(value)

for workspace in entries:
    pkgfile = root / workspace / "package.json"
    if not pkgfile.exists():
        continue

    data = json.loads(pkgfile.read_text())
    if data.get("private"):
        continue

    name = data["name"]
    version = data["version"]

    pkg = subprocess.run(
        ["npm", "view", name, "version"],
        text=True,
        capture_output=True,
    )
    ver = subprocess.run(
        ["npm", "view", f"{name}@{version}", "version"],
        text=True,
        capture_output=True,
    )

    if pkg.returncode != 0 or ver.returncode != 0:
        print(f"{name}@{version}")
        print(f"  package rc={pkg.returncode} latest={pkg.stdout.strip()!r}")
        if pkg.stderr.strip():
            print(f"  package stderr={pkg.stderr.strip().splitlines()[-1]!r}")
        print(f"  version rc={ver.returncode} published={ver.stdout.strip()!r}")
        if ver.stderr.strip():
            print(f"  version stderr={ver.stderr.strip().splitlines()[-1]!r}")
PY

Repository: thefrontside/effectionx

Length of output: 49


🏁 Script executed:

cat -n .internal/check-versions.ts

Repository: thefrontside/effectionx

Length of output: 4469


🏁 Script executed:

head -60 .internal/publish-matrix.ts

Repository: thefrontside/effectionx

Length of output: 1719


🏁 Script executed:

rg -n "versions:sync|check-versions" --type ts

Repository: thefrontside/effectionx

Length of output: 49


Don't treat every failed npm view as "version not published" — distinguish package existence from transient failure.

The code conflates three distinct cases: version genuinely missing, package never published, and registry/auth/network failure. The last should hard-fail rather than cascade, but this design also loses the first-publish distinction already tracked in .internal/publish-matrix.ts:20-50. In versions:sync mode, a transient npm failure triggers version rewrites on unrelated packages (lines 66-88). Additionally, the cascade failure explanation (lines 101-110) can report empty trigger strings when conditions align.

Unlike publish-matrix.ts, which runs separate npm view ${name} and npm view ${name}@${version} checks to distinguish package existence, check-versions.ts runs only the version check. Any non-zero exit code becomes a false positive pending release.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.internal/check-versions.ts around lines 28 - 39, The loop over
graph.packages currently treats any non-zero exit from the `npm view
${pkg.name}@${pkg.version}` call (npmCheck/result) as "version not published"
and adds pkg.name to pendingRelease; change this to distinguish three cases: (1)
run `npm view ${pkg.name}@${pkg.version}` and if it returns exitCode 0 with
stdout non-empty, do nothing; (2) if version-check exitCode !== 0, run a plain
`npm view ${pkg.name}` to see if the package exists—if that exists but version
missing, add to pendingRelease; (3) if the package-check itself failed with a
transient/network/auth error (non-zero exitCode or other error), surface a hard
failure (throw or exit) instead of adding to pendingRelease so `versions:sync`
cannot rewrite unrelated packages; mirror the existence/version split logic used
in publish-matrix.ts and use the same helper/command patterns around
npmCheck/result and pendingRelease to locate and update the behavior.

Comment on lines +101 to +115
// Group by the dependency that triggered the cascade.
for (const name of requiredBumps) {
const pkg = graph.packages.get(name)!;
// Find which of its deps triggered this.
const triggers = pkg.publishedWorkspaceDeps.filter((dep) =>
pendingRelease.has(dep),
);
const triggerStr = triggers
.map((t) => {
const tp = graph.packages.get(t)!;
return `${t}@${tp.version}`;
})
.join(", ");

console.error(` ${name} (${pkg.version}) — depends on ${triggerStr}`);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Preserve the trigger chain for transitive failures.

For multi-hop cascades, triggers only checks pendingRelease, so packages reached via another required bump can print depends on with no cause. That makes the check-mode output hard to act on for the exact transitive cases this tool is supposed to catch.

🛠️ Minimal fix
-      const triggers = pkg.publishedWorkspaceDeps.filter((dep) =>
-        pendingRelease.has(dep),
-      );
+      const triggers = pkg.publishedWorkspaceDeps.filter(
+        (dep) => pendingRelease.has(dep) || requiredBumps.has(dep),
+      );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Group by the dependency that triggered the cascade.
for (const name of requiredBumps) {
const pkg = graph.packages.get(name)!;
// Find which of its deps triggered this.
const triggers = pkg.publishedWorkspaceDeps.filter((dep) =>
pendingRelease.has(dep),
);
const triggerStr = triggers
.map((t) => {
const tp = graph.packages.get(t)!;
return `${t}@${tp.version}`;
})
.join(", ");
console.error(` ${name} (${pkg.version}) — depends on ${triggerStr}`);
// Group by the dependency that triggered the cascade.
for (const name of requiredBumps) {
const pkg = graph.packages.get(name)!;
// Find which of its deps triggered this.
const triggers = pkg.publishedWorkspaceDeps.filter(
(dep) => pendingRelease.has(dep) || requiredBumps.has(dep),
);
const triggerStr = triggers
.map((t) => {
const tp = graph.packages.get(t)!;
return `${t}@${tp.version}`;
})
.join(", ");
console.error(` ${name} (${pkg.version}) — depends on ${triggerStr}`);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.internal/check-versions.ts around lines 101 - 115, The current grouping
uses pkg.publishedWorkspaceDeps filtered only by pendingRelease, which loses the
transitive cause for packages in requiredBumps; change the trigger detection in
the loop over requiredBumps to walk dependency edges (starting from each dep in
pkg.publishedWorkspaceDeps) back to the nearest package that is in
pendingRelease (or is an original root in requiredBumps) and record that root
trigger so the printed triggerStr shows the actual triggering package and
version; update the logic around requiredBumps / pkg.publishedWorkspaceDeps /
pendingRelease (the block building triggers and triggerStr) to perform this
traversal and map each dependency to its ultimate triggering package before
formatting the output.

Comment on lines +43 to +56
const workspaceYaml = yield* call(() =>
fsp.readFile(resolve(rootDir, "pnpm-workspace.yaml"), "utf-8"),
);

const workspaces: string[] = [];
for (const line of workspaceYaml.split("\n")) {
const trimmed = line.trim();
if (trimmed.startsWith("-")) {
const value = trimmed.replace(/^-\s*/, "").replace(/^["']|["']$/g, "");
if (value) {
workspaces.push(value);
}
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Share the workspace parser with sync-tsrefs instead of copying it.

This is now a second hand-rolled pnpm-workspace.yaml parser, but it omits the glob-character guard already present in .internal/sync-tsrefs.ts:56-96. The two internal tools can now disagree on valid workspace entries and fail with different errors.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.internal/lib/dep-graph.ts around lines 43 - 56, This code duplicates a
pnpm-workspace.yaml parser and omits the glob guard; instead reuse the existing
parser from .internal/sync-tsrefs.ts: export the parser there (e.g.,
parsePnpmWorkspace or parseWorkspaceYaml) and import it into
.internal/lib/dep-graph.ts, replacing the manual loop that builds workspaces
from workspaceYaml and the local workspaces array; ensure the shared parser
preserves the glob-character guard and returns the same string[] shape so
dep-graph.ts can use it without additional logic.

Comment on lines +145 to +163
/** Extract workspace package names that use `workspace:*` protocol. */
function collectWorkspaceDeps(
depsObject: Record<string, string> | undefined,
packageNames: Set<string>,
): string[] {
if (!depsObject || typeof depsObject !== "object") {
return [];
}

const result: string[] = [];
for (const [name, range] of Object.entries(depsObject)) {
if (
packageNames.has(name) &&
typeof range === "string" &&
range.startsWith("workspace:")
) {
result.push(name);
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
python - <<'PY'
import json
import pathlib

root = pathlib.Path(".")
entries = []

workspace_file = root / "pnpm-workspace.yaml"
for line in workspace_file.read_text().splitlines():
    trimmed = line.strip()
    if trimmed.startswith("-"):
        value = trimmed[1:].strip().strip('"').strip("'")
        if value:
            entries.append(value)

for workspace in entries:
    pkgfile = root / workspace / "package.json"
    if not pkgfile.exists():
        continue

    data = json.loads(pkgfile.read_text())
    for section in ("dependencies", "peerDependencies"):
        deps = data.get(section) or {}
        if not isinstance(deps, dict):
            continue
        for name, rng in deps.items():
            if isinstance(rng, str) and rng.startswith("workspace:") and rng != "workspace:*":
                print(f"{pkgfile}: {section}.{name} = {rng}")
PY

Repository: thefrontside/effectionx

Length of output: 49


Code doesn't match documented policy scope for workspace dependency cascade.

The startsWith("workspace:") predicate is broader than the documented workspace:* cascade rule (which covers workspace:^, workspace:~, and other variants). While the repository currently has no non-workspace:* workspace dependencies that would trigger unnecessary bumps, the code should be narrowed to align with the policy:

-      range.startsWith("workspace:")
+      range === "workspace:*"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.internal/lib/dep-graph.ts around lines 145 - 163, The predicate in
collectWorkspaceDeps is too broad (range.startsWith("workspace:")); narrow it to
only match the documented workspace cascade variants (e.g., workspace:*,
workspace:^, workspace:~) by replacing that check with a strict pattern test on
range (for example using a regex like /^workspace:(\*|~|\^)/) while keeping the
existing guards (depsObject, typeof range) and leaving packageNames and result
handling intact.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.internal/check-versions.ts:
- Around line 78-85: The current code reparses and reserializes package.json
which can reorder fields and change formatting; instead update only the version
field in the original raw text to preserve ordering/formatting: locate the
"version" property in the raw string (e.g., using a regex that matches the
"version" key and its value while preserving surrounding whitespace/commas),
replace just the value with newVersion, and then pass the modified raw string to
writeTextFile; fall back to the existing JSON.parse/JSON.stringify approach
(using packageJsonPath, readTextFile, writeTextFile, newVersion) only if the
targeted text-replacement fails.

In @.internal/lib/dep-graph.ts:
- Around line 63-64: The code currently calls
readTextFile(resolve(workspacePath, "package.json")) twice (variables raw/json)
during the workspace scan and again for non-private packages; change the flow so
the initial parse (the json object produced after JSON.parse(raw) in the first
pass) is saved and reused instead of re-reading the file—e.g., store the parsed
object in a local variable scoped outside the condition or attach it to the
workspace record, then reference that parsed JSON when checking package privacy
and other fields (avoid a second readTextFile call around lines where
package.json is read again).
- Around line 66-68: The code pushes package entries assuming json.name and
json.version are valid strings (see allPackages push in dep-graph.ts), which can
break on malformed package.json; add validation before pushing: validate that
json.name and json.version exist and are non-empty strings (or use the existing
zod dependency to define a schema and parse json), and if validation fails skip
the package or throw a clear error (include the package path/context in the
message) so downstream consumers of allPackages only receive well-formed {name,
version} entries.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9176c96e-4465-4c78-9b9f-56ee41bd43a2

📥 Commits

Reviewing files that changed from the base of the PR and between 9a09857 and 098dad4.

📒 Files selected for processing (2)
  • .internal/check-versions.ts
  • .internal/lib/dep-graph.ts

Comment on lines +78 to +85
const packageJsonPath = resolve(pkg.workspacePath, "package.json");
const raw = yield* readTextFile(packageJsonPath);
const json = JSON.parse(raw) as Record<string, unknown>;
json.version = newVersion;
yield* writeTextFile(
packageJsonPath,
`${JSON.stringify(json, null, 2)}\n`,
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

JSON serialization may alter field ordering and formatting.

JSON.stringify with 2-space indent is standard, but it won't preserve the original field order or any custom formatting (like trailing commas if present). This is generally acceptable for package.json, but be aware that diffs may show reordering.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.internal/check-versions.ts around lines 78 - 85, The current code reparses
and reserializes package.json which can reorder fields and change formatting;
instead update only the version field in the original raw text to preserve
ordering/formatting: locate the "version" property in the raw string (e.g.,
using a regex that matches the "version" key and its value while preserving
surrounding whitespace/commas), replace just the value with newVersion, and then
pass the modified raw string to writeTextFile; fall back to the existing
JSON.parse/JSON.stringify approach (using packageJsonPath, readTextFile,
writeTextFile, newVersion) only if the targeted text-replacement fails.

Comment on lines +63 to +64
const raw = yield* readTextFile(resolve(workspacePath, "package.json"));
const json = JSON.parse(raw) as Record<string, unknown>;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Avoid reading package.json twice for non-private packages.

The file is read once at line 63 during the initial workspace scan, then read again at line 90 for non-private packages. Store the parsed JSON in the first pass to avoid redundant I/O.

♻️ Suggested approach
   // Read all package.json files and build WorkspacePackage entries.
   const allPackages: WorkspacePackage[] = [];
+  const packageJsonCache = new Map<string, Record<string, unknown>>();

   for (const workspace of workspaces) {
     const workspacePath = resolve(rootDir, workspace);
     const raw = yield* readTextFile(resolve(workspacePath, "package.json"));
     const json = JSON.parse(raw) as Record<string, unknown>;

+    packageJsonCache.set(workspace, json);
     allPackages.push({
       name: json.name as string,
       // ...
     });
   }

   // Later, when resolving deps:
   for (const pkg of packages.values()) {
-    const raw = yield* readTextFile(resolve(pkg.workspacePath, "package.json"));
-    const json = JSON.parse(raw) as Record<string, unknown>;
+    const json = packageJsonCache.get(pkg.workspace)!;

     const deps = collectWorkspaceDeps(
       json.dependencies as Record<string, string> | undefined,
       packageNames,
     );

Also applies to: 89-91

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.internal/lib/dep-graph.ts around lines 63 - 64, The code currently calls
readTextFile(resolve(workspacePath, "package.json")) twice (variables raw/json)
during the workspace scan and again for non-private packages; change the flow so
the initial parse (the json object produced after JSON.parse(raw) in the first
pass) is saved and reused instead of re-reading the file—e.g., store the parsed
object in a local variable scoped outside the condition or attach it to the
workspace record, then reference that parsed JSON when checking package privacy
and other fields (avoid a second readTextFile call around lines where
package.json is read again).

Comment on lines +66 to +68
allPackages.push({
name: json.name as string,
version: json.version as string,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Missing validation for required name and version fields.

The code assumes json.name and json.version exist and are strings. A malformed package.json (or workspace pointing to a non-package directory) would cause subtle failures downstream. Add validation or use a schema library like zod (already a dependency per manifest).

🛡️ Suggested validation
+    if (typeof json.name !== "string" || !json.name) {
+      throw new Error(`Missing or invalid "name" in ${workspacePath}/package.json`);
+    }
+    if (typeof json.version !== "string" || !json.version) {
+      throw new Error(`Missing or invalid "version" in ${workspacePath}/package.json`);
+    }
+
     allPackages.push({
       name: json.name as string,
       version: json.version as string,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.internal/lib/dep-graph.ts around lines 66 - 68, The code pushes package
entries assuming json.name and json.version are valid strings (see allPackages
push in dep-graph.ts), which can break on malformed package.json; add validation
before pushing: validate that json.name and json.version exist and are non-empty
strings (or use the existing zod dependency to define a schema and parse json),
and if validation fails skip the package or throw a clear error (include the
package path/context in the message) so downstream consumers of allPackages only
receive well-formed {name, version} entries.

@cowboyd
Copy link
Copy Markdown
Member

cowboyd commented Mar 13, 2026

What about using convector here?

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.

2 participants