feat(workspace): manage pnpm workspaces with Bit, on the workspace-root component - #10729
Open
davidfirst wants to merge 26 commits into
Open
davidfirst wants to merge 26 commits into
davidfirst wants to merge 26 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… component "bit pnpm sync" now tracks each pnpm project as a component and the root with rootDir ".", under trackAllFiles, instead of explicit file lists and a topology in tracker config. Drops the --pnpm-vcs-root/--pnpm-vcs-bootstrap flags, toolchain profiles, the vcs block in pnpm-workspace.yaml, the manifest migration and the package-scripts env (CI runs the scripts after "bit clone"). The import plan no longer rewrites package manifests. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ionless packages componentIdToPackageName now honors the dependency-resolver packageName config, so the linker and the root-components linking agree on the name. A found package with no version (a private pnpm package) is recorded as "*" instead of failing the dependency detection. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ce refs on import Adds teambit.envs/pnpm-workspace-env, a plain-JS env on the empty env that compiles and builds through the packages' own scripts, in a pnpm workspace rebuilt from the workspace-root component. "bit pnpm sync" assigns it to projects with scripts (--env to override). On import, a "workspace:" reference to a sibling the target workspace lacks becomes "catalog:", bound to its exact version. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
PR Summary by QodoManage pnpm workspaces through workspace-root components
AI Description
Diagram
High-Level Assessment
Files changed (36)
|
Code Review by Qodo
1. One test fixture fails formatting
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
…nd clone In a pnpm-adopted workspace, the component writer and bit clone no longer write dependencies into the root package.json or run bit's install and compile, which would also run the packages' build scripts. They hint to run "pnpm install" instead. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 920cefc |
- mark each synced project in its tracker config, so a later sync removes only its own components (nameless ones included) and can move one from the env it assigned to another --env - drop a package name the package.json no longer gives - read the default catalog by the same rule as import writes it - clear a capsule's output dirs before copying a new build's output - list only removals and a summary in the sync report, with the error symbol for removals - skip schemaVersion in the e2e clean-status check Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Comment on lines
+724
to
+727
| const dependency = dependencies.findByPkgNameOrCompId(dependencyName); | ||
| // declared in package.json but never imported by the code, so bit recorded no dependency - and no | ||
| // version to bind. the entry stays as the pnpm manifest has it, which is versioned with the root. | ||
| if (!dependency) continue; |
There was a problem hiding this comment.
1. Selective imports can fail to install 🐞 Bug ≡ Correctness
createPnpmVcsImportPlan() skips a missing local workspace: reference when findByPkgNameOrCompId() cannot find that package in Bit's detected dependency data. Importing a component without that sibling then leaves the manifest's workspace-only specifier unchanged, so pnpm cannot resolve it in the target workspace.
Agent Prompt
## Issue description
Selective imports leave `workspace:` dependencies unchanged when Bit did not detect the declared package, producing a manifest pnpm cannot install when the sibling is absent.
## Fix Focus Areas
- scopes/component/tracker/pnpm-vcs-sync.cmd.ts[719-747]
- scopes/component/tracker/pnpm-vcs-sync.cmd.ts[496-523]
## Recommended Fix
When a missing local `workspace:` dependency has no detected exact version, do not silently omit it from the plan. Either derive a valid exact binding from persisted component metadata or fail the import before writing files with an actionable error identifying the unresolved package.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+96
to
+97
| lifecycle: 'runtime' | 'dev'; | ||
| optional: boolean; |
There was a problem hiding this comment.
2. Peer components lose peer semantics 🐞 Bug ≡ Correctness
GraphComponentDependency.lifecycle permits only runtime and development values, and _promoteGraphComponentDependencies() consequently has no peer-dependency target. A pnpm workspace component declared as a peer is therefore omitted from graph promotion or retained under the wrong lifecycle when its source imports that package, including for optional peers.
Agent Prompt
## Issue description
The new graph promotion model cannot represent peer dependencies, so pnpm workspace component peers are not persisted with their declared lifecycle and optional metadata.
## Fix Focus Areas
- scopes/component/snapping/version-maker.ts[93-123]
- scopes/component/snapping/version-maker.ts[391-412]
- scopes/dependencies/pnpm/lockfile-deps-graph-converter.ts[64-105]
## Recommended Fix
Extend graph conversion and promotion to represent peer lifecycle explicitly, route promoted peers to `component.peerDependencies`, and preserve `peerDependenciesMeta.optional` in dependency-resolver data.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+515
to
+519
| for (const componentMap of this.consumer.bitMap.components) { | ||
| const config = componentMap.config?.[DependencyResolverAspect.id]; | ||
| const configuredPackageName = config && config !== '-' ? config.packageName : undefined; | ||
| if (typeof configuredPackageName === 'string' && configuredPackageName) { | ||
| result.set(configuredPackageName, componentMap.id); |
There was a problem hiding this comment.
3. Imports can target unavailable components 🐞 Bug ≡ Correctness
getWorkspaceComponentIdsByPackageName() indexes every bitmap entry without checking whether the component is removed or available on the current lane. When a stale entry exposes the same package name as a resolved package import, processPackages() replaces that package with a dependency on the unavailable component and deletes the valid package dependency.
Agent Prompt
## Issue description
The new package-name lookup includes removed and other-lane bitmap entries, allowing dependency detection to promote installed packages to components unavailable in the current workspace state.
## Fix Focus Areas
- scopes/dependencies/dependencies/dependencies-loader/auto-detect-deps.ts[512-535]
- components/legacy/bit-map/bit-map.ts[244-263]
## Recommended Fix
Populate the package-name map only from components that are available on the current lane and are not removed. Also detect duplicate active package names rather than silently overwriting an earlier mapping.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 1916412 |
…s load as ESM Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Comment on lines
+37
to
+38
| export function isPnpmWorkspaceRoot(workspacePath: string, bitMap: BitMap): boolean { | ||
| return Boolean(findWorkspaceRootMap(bitMap)) && fs.existsSync(path.join(workspacePath, PNPM_WORKSPACE_MANIFEST)); |
There was a problem hiding this comment.
1. Ordinary workspaces enter pnpm mode 🐞 Bug ≡ Correctness
isPnpmWorkspaceRoot() and isPnpmWorkspace() treat any workspace-root component beside pnpm-workspace.yaml as proof that the workspace was adopted by bit pnpm sync, without checking durable adoption state. An ordinary Bit workspace using pnpm and a root component is therefore routed through pnpm-specific behavior, causing component writes and clones to skip Bit installation and compilation while imports may modify its pnpm manifest.
Agent Prompt
## Issue description
Pnpm workspace mode is inferred from the presence of a workspace-root component and `pnpm-workspace.yaml`, a combination ordinary Bit workspaces can also have. This causes installation, compilation, cloning, and import behavior to change even when `bit pnpm sync` never adopted the workspace.
## Fix Focus Areas
- scopes/workspace/workspace-root/workspace-root-data.ts[31-39]
- scopes/component/tracker/pnpm-vcs-sync.cmd.ts[176-180]
- scopes/component/tracker/pnpm-vcs-sync.cmd.ts[333-347]
## Recommended Fix
Persist an explicit, versioned pnpm-adoption marker on the workspace-root component during sync, and require that marker in both pnpm-workspace predicates instead of inferring adoption from the manifest’s existence. Ensure the marker survives cloning through the versioned bitmap.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+333
to
+335
| async function trackPnpmWorkspaceRoot(workspace: Workspace, tracker: TrackerMain): Promise<ComponentID> { | ||
| const existingId = workspace.consumer.bitMap.getComponentIdByRootPath(WORKSPACE_ROOT_DIR); | ||
| if (existingId) return existingId; |
There was a problem hiding this comment.
2. Workspace roots keep stale file lists 🐞 Bug ≡ Correctness
trackPnpmWorkspaceRoot() returns an existing root ID without invoking the tracker scan that recomputes the root component’s files after nested component directories change. On subsequent syncs, newly added projects can remain listed in both their component and the root, removed-project files remain stale, and new root-level files are omitted from the root’s next snapshot.
Agent Prompt
## Issue description
An existing workspace-root component bypasses tracking during subsequent pnpm syncs, so its file inventory is never reconciled with current project directories or root files. The root can consequently snapshot stale, missing, or multiply owned files.
## Fix Focus Areas
- scopes/component/tracker/pnpm-vcs-sync.cmd.ts[333-347]
- scopes/component/tracker/add-components.ts[1009-1029]
## Recommended Fix
Rescan and update the existing workspace-root component on every sync after project additions and removals, while preserving its existing component ID and root configuration. Add rerun coverage for adding a project, removing a project, and adding a root-level file.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+707
to
+709
| const workspaceManifest = await readPnpmWorkspaceManifest(path.join(workspace.path, PNPM_WORKSPACE_MANIFEST)); | ||
| const localPackageNames = await readLocalPackageNames(workspace.path, workspaceManifest.packages || []); | ||
| plannedComponents.forEach(({ packageName }) => localPackageNames.add(packageName)); |
There was a problem hiding this comment.
3. Imports create duplicate workspace packages 🐞 Bug ≡ Correctness
createPnpmVcsImportPlan() collects imported package names into localPackageNames with Set.add() but never rejects names that already belong to a local or another imported project. applyPnpmImportPlan() then adds each imported root directory to pnpm-workspace.yaml, whereas the sync path explicitly validates duplicate project identities before tracking them.
Agent Prompt
## Issue description
Import planning silently accepts package names that collide with an existing workspace project or another imported component, then adds all corresponding directories to the pnpm workspace.
## Fix Focus Areas
- scopes/component/tracker/pnpm-vcs-sync.cmd.ts[694-709]
- scopes/component/tracker/pnpm-vcs-sync.cmd.ts[430-463]
## Recommended Fix
Before returning the import plan, validate that every planned package name is unique and does not collide with package names already discovered in the target workspace. Throw a clear `BitError` identifying the duplicate package name and conflicting component or directory; add coverage for collisions with both local and batch-imported projects.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+120
to
+122
| await writePnpmWorkspaceTree(tree, treeDir); | ||
| await this.pnpm(['install', '--frozen-lockfile'], treeDir); | ||
| await fs.writeFile(markerPath, signature); |
There was a problem hiding this comment.
14. Build tasks reject lockfile-free workspaces 🐞 Bug ☼ Reliability
PnpmScriptTask.prepareTree() always runs pnpm install --frozen-lockfile after reconstructing the workspace tree. Sync accepts a workspace based only on pnpm-workspace.yaml, so an adopted workspace without a committed lockfile reaches that frozen installation before any build, test, or lint script can run.
Agent Prompt
## Issue description
The isolated workspace tree is always installed with `--frozen-lockfile`, even when its reconstructed root contains no lockfile, causing every package-script task to fail before scripts run.
## Fix Focus Areas
- scopes/envs/pnpm-workspace-env/pnpm-script.task.js[115-122]
- scopes/envs/pnpm-workspace-env/pnpm-workspace-tree.js[157-165]
## Recommended Fix
Detect whether `pnpm-lock.yaml` exists in the reconstructed tree. Keep `--frozen-lockfile` when it exists, but perform a normal pnpm install when it does not so pnpm can generate the initial lockfile for the isolated build tree; add a test for a workspace root without a lockfile.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit d8b6c28 |
…napped with, one install hint on clone Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| interactiveConfig?.generator || generator, | ||
| agentType, | ||
| { skipDefaultMcp: userOptedOutOfMcp } | ||
| { skipAgentInstructions: Boolean(noAgent), skipDefaultMcp: Boolean(noMcp) || userOptedOutOfMcp } |
There was a problem hiding this comment.
1. Init opt-outs still create files 🐞 Bug ≡ Correctness
handleInteractiveMode() writes the selected MCP configuration and agent rules before report() passes noAgent and noMcp to the initializer. In an interactive Git-repository setup, selecting an MCP editor therefore creates files even when the corresponding command-line opt-out was supplied.
Agent Prompt
## Issue description
Interactive initialization writes MCP configuration and agent rules before the new `--no-mcp` and `--no-agent` options reach the main initializer, making those opt-outs ineffective.
## Fix Focus Areas
- scopes/harmony/host-initializer/init-cmd.ts[78-119]
- scopes/harmony/host-initializer/init-cmd.ts[149-212]
## Recommended Fix
Pass the opt-out flags into `handleInteractiveMode()` and prevent `setupMcpServer()` when `noMcp` is set and `writeMcpAgentRules()` when `noAgent` is set. Preserve the flags when invoking `HostInitializerMain.init()` so both interactive and non-interactive paths honor them.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+124
to
+125
| const stat = await fs.stat(fullPath); | ||
| entries.push(`${path.relative(workspaceDir, fullPath)}:${stat.size}:${stat.mtimeMs}`); |
There was a problem hiding this comment.
16. Some source edits keep stale builds 🐞 Bug ☼ Reliability
sourceSignature() represents each source file using only its path, size, and modification timestamp, while buildIfSourcesChanged() skips the package build whenever that metadata matches its cache. An edit that preserves both size and timestamp, such as a metadata-preserving checkout or filesystem with coarse timestamp resolution, is treated as unchanged and leaves the previous compiled output in place.
Agent Prompt
## Issue description
The workspace compiler's cache key uses file size and modification time rather than file contents, allowing changed sources to reuse stale build output.
## Fix Focus Areas
- scopes/envs/pnpm-workspace-env/pnpm-workspace.compiler.js[84-91]
- scopes/envs/pnpm-workspace-env/pnpm-workspace.compiler.js[111-130]
## Recommended Fix
Build the source signature from stable relative paths and file-content hashes instead of size and timestamp metadata. Retain the existing excluded-directory filtering and deterministic path ordering.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+90
to
+94
| const treeDir = path.join(context.capsuleNetwork.capsulesRootDir, TREE_DIR); | ||
| try { | ||
| await this.prepareTree(tree, treeDir); | ||
| await this.pnpm(['-r', '--if-present', 'run', this.script], treeDir); | ||
| await copyOutputsToCapsules(context, tree, treeDir, OUTPUT_DIRS[this.script]); |
There was a problem hiding this comment.
2. Concurrent builds corrupt pnpm workspaces 🐞 Bug ☼ Reliability
PnpmScriptTask.execute() derives treeDir solely from the shared capsule root and the fixed .pnpm-workspace name, while prepareTree() removes, rewrites, installs, and marks that directory. When build concurrency runs distinct environments in parallel, their tasks can overwrite the same reconstructed tree while scripts and output copying are in progress.
Agent Prompt
## Issue description
All pnpm script tasks reconstruct their workspace at the same `.pnpm-workspace` directory below `capsulesRootDir`. The build scheduler may execute tasks from different environments concurrently, so one task can remove or rewrite the tree while another is installing dependencies, running a script, or copying outputs.
## Fix Focus Areas
- scopes/envs/pnpm-workspace-env/pnpm-script.task.js[90-94]
- scopes/envs/pnpm-workspace-env/pnpm-script.task.js[115-122]
## Recommended Fix
Derive the reconstructed workspace path from a stable environment-specific value as well as the capsule root, such as a sanitized/hash-safe `context.envId`. Ensure build, test, lint, installation-marker, and output-copy operations for an environment use that same private directory, preventing tasks from separate environments from sharing mutable state.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit ed5ef76 |
Comment on lines
+113
to
+117
| const componentId = componentIdByPkgName.get(neighbour.name); | ||
| if (!componentId) continue; | ||
| graph.packages.set(neighbour.id, { | ||
| component: { scope: componentId.scope, name: componentId.fullName }, | ||
| version: componentId.version ? snapToSemver(componentId.version) : undefined, |
There was a problem hiding this comment.
1. Snaps replace registry dependencies 🐞 Bug ≡ Correctness
addWorkspaceComponentPackages() overwrites every root graph package whose name matches a workspace component, even when the lockfile reference resolved to an ordinary registry package. A snap with a same-named local project therefore records the local component identity and version instead of the dependency selected by the package manager.
Agent Prompt
## Issue description
The dependency graph relabels registry-resolved packages as workspace components based solely on matching package names.
## Fix Focus Areas
- scopes/dependencies/pnpm/lockfile-deps-graph-converter.ts[88-123]
## Recommended Fix
Track which neighbours were created from verified `link:` or `workspace:` references and attach component metadata only to those neighbours. Preserve the existing lockfile package metadata for registry references, even when their names match workspace components.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+119
to
+120
| if (dirent.isDirectory()) { | ||
| if (!NON_SOURCE_DIRS.has(dirent.name)) await walk(fullPath); |
There was a problem hiding this comment.
16. Source edits can skip compilation 🐞 Bug ≡ Correctness
sourceSignature() refuses to descend into every directory named lib or build, although those names can contain authored source rather than generated output. After one successful compilation, changing such a file leaves the cached signature unchanged and buildIfSourcesChanged() returns without running the package build script.
Agent Prompt
## Issue description
Incremental compilation excludes directories by basename, causing legitimate source changes under names such as `lib` and `build` to reuse stale build results.
## Fix Focus Areas
- scopes/envs/pnpm-workspace-env/pnpm-workspace.compiler.js[7-9]
- scopes/envs/pnpm-workspace-env/pnpm-workspace.compiler.js[84-91]
- scopes/envs/pnpm-workspace-env/pnpm-workspace.compiler.js[111-130]
## Recommended Fix
Compute the signature from tracked source files or exclude generated output only at known package output paths rather than skipping every directory with these basenames. Ensure changes beneath legitimate `lib` or `build` source directories alter the signature and rerun the build.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+141
to
+143
| directDependencies.push( | ||
| ...importerDepsToNeighbours(componentDevImporter.devDependencies, 'dev', false, componentIdByPkgName) | ||
| ); |
There was a problem hiding this comment.
2. Workspace package versions are lost 🐞 Bug ≡ Correctness
convertLockfileToGraph() passes componentIdByPkgName only while converting the project importer’s development dependencies, then its component-root runtime loop drops every non-registry link: or workspace: reference when refToRelative() returns null. This occurs when a component is resolved through a root-components importer, so snapping can persist no component dependency for a runtime pnpm workspace package and later installs do not receive the required exact component version.
Agent Prompt
## Issue description
The component-root conversion path handles `link:` and `workspace:` references for development importer dependencies, but not for runtime or optional dependencies read from the locked package snapshot. As a result, direct runtime workspace-component dependencies can be omitted from the graph that snapping promotes into persisted Bit dependencies.
## Fix Focus Areas
- scopes/dependencies/pnpm/lockfile-deps-graph-converter.ts[141-162]
## Recommended Fix
Apply the same `componentIdByPkgName` fallback used by `importerDepsToNeighbours()` to the runtime and optional dependency loop. When a snapshot reference is a workspace-style non-registry reference and its package name maps to a component, create the snap-semver neighbour and ensure its package attributes identify that target as a component; retain the current behavior for unrelated non-registry references. Add coverage for the `componentRootDir` path with a runtime workspace dependency.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit ed5ef76 |
…en switching to a lane Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… rewrites Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
…atalog entries an import wrote Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Comment on lines
+467
to
+468
| const uncoveredRootDirs = plan.components.map(({ rootDir }) => rootDir).filter((dir) => !coveredRootDirs.has(dir)); | ||
| if (uncoveredRootDirs.length) manifest.packages = [...(manifest.packages || []), ...uncoveredRootDirs]; |
There was a problem hiding this comment.
1. Imported projects remain outside pnpm 🐞 Bug ≡ Correctness
applyPnpmImportPlan() appends an imported root to packages without removing an existing negated pattern that matches the same directory. When a workspace excludes that root, both readLocalPackageNames() and the next syncPnpmWorkspace() use discoverPnpmProjectManifests()'s unconditional ignore list, so the imported package is neither locally bound nor synchronized as a pnpm project.
Agent Prompt
## Issue description
Imported component roots are appended to `pnpm-workspace.yaml`, but matching negated package patterns still exclude them from pnpm discovery and later synchronization.
## Fix Focus Areas
- scopes/component/tracker/pnpm-vcs-sync.cmd.ts[462-468]
- scopes/component/tracker/pnpm-vcs-sync.cmd.ts[560-575]
## Recommended Fix
When applying an import plan, reconcile matching negated patterns so every imported root is effectively included. Remove or narrow exclusions that match imported roots, or introduce discovery semantics that explicitly allow imported roots to override those exclusions, and add coverage for importing into a previously excluded directory.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+141
to
+143
| directDependencies.push( | ||
| ...importerDepsToNeighbours(componentDevImporter.devDependencies, 'dev', false, componentIdByPkgName) | ||
| ); |
There was a problem hiding this comment.
2. Dev workspace dependencies disappear after snaps 🐞 Bug ≡ Correctness
convertLockfileToGraph() creates graph neighbours for link: and workspace: development dependencies but does not call addWorkspaceComponentPackages() in its normal component-root path. When a pnpm project is resolved through the root-components importer, those synthetic neighbours have no package metadata, so componentDependenciesFromGraph() skips them and the snapped component does not retain its development dependency.
Agent Prompt
## Issue description
The normal `convertLockfileToGraph()` path now preserves `link:` and `workspace:` development dependencies as root neighbours, but it does not add the corresponding workspace component metadata to `graph.packages`. Dependency promotion requires that metadata, so these development dependencies are omitted from the snapped component.
## Fix Focus Areas
- scopes/dependencies/pnpm/lockfile-deps-graph-converter.ts[125-184]
- scopes/dependencies/pnpm/lockfile-deps-graph-converter.ts[109-123]
## Recommended Fix
After `_convertLockfileToGraph()` returns in the normal component-root branch, call `addWorkspaceComponentPackages(graph, componentIdByPkgName)` before returning the graph, as the capsule conversion branch already does. Add coverage for a development `link:` or `workspace:` dependency in the component-root importer path and assert that its root neighbour has component metadata.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 913c006 |
…e import handling and the env Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| name: '@acme/app', | ||
| main: 'dist/index.js', | ||
| dependencies: { '@acme/math': 'workspace:*' }, | ||
| scripts: { build: 'node build.js', test: "node -e \"require('./dist') === 3 && console.log('app works')\"" }, |
There was a problem hiding this comment.
1. One test fixture fails formatting 📘 Rule violation ⚙ Maintainability
The scripts property in the app package fixture remains on one line beyond the configured 120-character print width. Running the canonical Prettier check over the E2E TypeScript files reformats this changed line, causing the formatting check to fail.
Agent Prompt
## Issue description
The app package fixture's `scripts` property does not conform to the repository's Prettier configuration and fails the canonical formatting check.
## Fix Focus Areas
- e2e/harmony/add-harmony.e2e.ts[637-637]
## Recommended Fix
Run Prettier on the file or manually expand the `scripts` object using Prettier's expected multiline layout without changing the fixture values.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| throw new BitError(`unable to find ${PNPM_WORKSPACE_MANIFEST} in ${workspace.path}`); | ||
| } | ||
| const workspaceManifest = await readPnpmWorkspaceManifest(workspaceManifestPath); | ||
| const projects = await discoverPnpmProjects(workspace.path, workspaceManifest.packages || []); |
There was a problem hiding this comment.
2. Sync removes every workspace project 🐞 Bug ≡ Correctness
syncPnpmWorkspace() converts an omitted packages field to an empty pattern list, even though the manifest type permits that field to be absent. Re-running sync on such a pnpm workspace discovers no projects and passes the empty inventory to removeLeftProjects(), which untracks unsnapped projects and marks snapped projects as removed.
Agent Prompt
## Issue description
`syncPnpmWorkspace()` treats an omitted `packages` field as an explicit empty list. This makes synchronization discover no projects and remove components synchronized by an earlier run.
## Fix Focus Areas
- scopes/workspace/pnpm-workspace/pnpm-workspace-sync.ts[139-143]
- scopes/workspace/pnpm-workspace/pnpm-workspace-sync.ts[193-212]
- scopes/workspace/pnpm-workspace/pnpm-workspace-sync.ts[559-578]
## Recommended Fix
When `packages` is undefined, pass pnpm's default project pattern, such as `['**']`, to discovery while preserving an explicitly configured empty array as empty. Add coverage distinguishing an omitted field from `packages: []`, including a repeat sync that verifies existing projects are not removed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+154
to
+157
| // a capsule is reused across builds, so what an earlier build left must not pass as this one's output | ||
| const target = path.join(capsule.path, outputDir); | ||
| await fs.rm(target, { recursive: true, force: true }); | ||
| const source = path.join(treeDir, rootDir, outputDir); |
There was a problem hiding this comment.
15. Other environments lose build outputs 🐞 Bug ≡ Correctness
copyOutputsToCapsules() iterates every member loaded from the workspace root and removes each matching capsule's output directories before checking whether the pnpm run produced replacements. When the capsule graph contains components outside context.components, this task can erase dist, build, or lib directories produced by another environment even during sequential task execution.
Agent Prompt
## Issue description
The pnpm task cleans output directories for every workspace member represented in the capsule graph, including components outside the environment currently executing the task. Outputs created by another environment can therefore be deleted without replacement.
## Fix Focus Areas
- scopes/workspace/pnpm-workspace/pnpm-script.task.ts[141-163]
- scopes/workspace/pnpm-workspace/pnpm-workspace-tree.ts[76-90]
## Recommended Fix
Build a set from `context.components` and restrict capsule output deletion and copying to tree members whose component IDs belong to that set. Add a test with an additional graph capsule outside the task context and verify its existing output directories remain unchanged.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+128
to
+129
| const stat = await fs.stat(fullPath); | ||
| entries.push(`${path.relative(workspaceDir, fullPath)}:${stat.size}:${stat.mtimeMs}`); |
There was a problem hiding this comment.
16. Source edits retain stale build output 🐞 Bug ≡ Correctness
sourceSignature() records only each file's path, size, and mtimeMs, and buildIfSourcesChanged() skips pnpm run build when that metadata string matches the prior successful build. When an edit preserves a source file's byte length and timestamp, such as through timestamp-restoring tooling, every transpileComponent() call for that workspace copies the prior dist output instead of recompiling.
Agent Prompt
## Issue description
The compiler cache key uses only source paths, file sizes, and modification timestamps. Content can change while retaining that metadata, causing the workspace build to be skipped and stale `dist` files to be copied to transpilation output.
## Fix Focus Areas
- scopes/workspace/pnpm-workspace/pnpm-workspace.compiler.ts[115-134]
## Recommended Fix
Make `sourceSignature()` content-sensitive by hashing each included source file's contents, along with its relative path, into a stable digest. Continue excluding generated and dependency directories, then use the digest as the `builtSignatures` cache value so an unchanged-size, unchanged-timestamp edit still triggers `pnpm run build`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 2ed18f1 |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on #10675, ported onto the workspace-root component model (#10698). What changed from the original, and why:
useExplicitFiles) for the root and projectsrootDir: "."plustrackAllFilespnpmVcs.workspace).bitmap, read throughWorkspaceRootMain.listMembers()bit import --pnpm-vcs-rootandbit switch --pnpm-vcs-bootstrapbit clone <root> [--lane]teambit.workspace/pnpm-workspaceaspect: it ownsbit pnpm syncand the import handling (through an importer slot), and is itself the scripts env, so the env ships with bit and needs no installDropped on purpose
engines.node/pnpm.vcs.profilechecks): they ran on import, but nothing else used their result.vcs:block inpnpm-workspace.yaml: the root component's.bitmapnow holds that identity.migratePnpmWorkspaceDependencies): it turnedworkspace:specifiers intocatalog:entries in the user's own package.json files. Adoption shouldn't edit those files.The import-side rewrite of
workspace:references tocatalog:is kept. It now applies only to siblings the target workspace doesn't have, so abit cloneleaves the manifests untouched.