Repository navigation
fix(fix): discover independent nested projects (UPT-120) #258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "uptide": patch | ||
| --- | ||
|
|
||
| Discover independent nested projects when fixing a dependency absent from the root project. Run installation, migration and verification in the selected project's directory, preserving its lockfile and package manager. When several projects declare the dependency, name their manifests and require an explicit directory. |
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import { rmSync } from 'node:fs'; | ||
| import { join } from 'node:path'; | ||
| import { afterEach, expect, it } from 'vitest'; | ||
| import { run } from './cli.js'; | ||
| import { fakeEngine, memoryIo, pnpmGitRepo } from './test-utils.js'; | ||
|
|
||
| const roots: string[] = []; | ||
| afterEach(() => { | ||
| for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| it('fix selects the nested project before dependency, manager and config preflight (UPT-120)', async () => { | ||
| const root = pnpmGitRepo({ | ||
| 'package.json': '{"name":"repo-tools","devDependencies":{"commitlint":"1.0.0"}}', | ||
| 'frontend/package.json': | ||
| '{"name":"frontend","packageManager":"npm@10.0.0","devDependencies":{"zod":"3.23.8"}}', | ||
| 'frontend/package-lock.json': '{"lockfileVersion":3}', | ||
| 'frontend/node_modules/zod/package.json': '{"name":"zod","version":"3.23.8"}', | ||
| 'frontend/uptide.yml': 'version: 1\nmaxCost: 2\n', | ||
| }); | ||
| roots.push(root); | ||
| const engine = fakeEngine(); | ||
| const io = memoryIo({ cwd: root }); | ||
| expect(await run(['fix', 'zod', '--no-llm'], io, engine), io.stderr()).toBe(0); | ||
| expect(engine.calls[0]).toMatchObject({ | ||
| cwd: join(root, 'frontend'), | ||
| only: 'zod', | ||
| maxCostUsd: 2, | ||
| }); | ||
| expect(io.stderr()).toContain('frontend'); | ||
| }); |
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; | ||
| import { tmpdir } from 'node:os'; | ||
| import { dirname, join } from 'node:path'; | ||
| import { afterEach, expect, it } from 'vitest'; | ||
| import { projectForDependency } from './projects.js'; | ||
|
|
||
| const roots: string[] = []; | ||
| afterEach(() => { | ||
| for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); | ||
| }); | ||
| function fixture(files: Record<string, string> = {}) { | ||
| const root = mkdtempSync(join(tmpdir(), 'uptide-projects-')); | ||
| roots.push(root); | ||
| for (const [file, contents] of Object.entries({ 'package.json': '{"name":"tools"}', ...files })) { | ||
| mkdirSync(dirname(join(root, file)), { recursive: true }); | ||
| writeFileSync(join(root, file), contents); | ||
| } | ||
| return root; | ||
| } | ||
| const manifest = '{"devDependencies":{"vitest":"^4.0.0"}}'; | ||
|
|
||
| it.each(['package-lock.json', 'npm-shrinkwrap.json', 'pnpm-lock.yaml', 'yarn.lock', 'bun.lock'])( | ||
| 'finds a nested install root using %s without installed dependencies', | ||
| (lock) => { | ||
| const root = fixture({ 'apps/frontend/package.json': manifest, [`apps/frontend/${lock}`]: '' }); | ||
| expect(projectForDependency(root, ['vitest'])).toBe(join(root, 'apps/frontend')); | ||
| }, | ||
| ); | ||
| it('preserves root and declared workspace selection', () => { | ||
| const cases: Record<string, string>[] = [ | ||
| { 'package.json': manifest }, | ||
| { 'package.json': '{"workspaces":["packages/*"]}', 'packages/ui/package.json': manifest }, | ||
| ]; | ||
| for (const files of cases) { | ||
| const root = fixture({ | ||
| ...files, | ||
| 'frontend/package.json': manifest, | ||
| 'frontend/package-lock.json': '', | ||
| }); | ||
| expect(projectForDependency(root, ['vitest'])).toBe(root); | ||
| } | ||
| }); | ||
| it('selects a nested workspace root rather than its member with a stray lockfile', () => { | ||
| const root = fixture({ | ||
| 'frontend/package.json': '{"workspaces":["packages/*"]}', | ||
| 'frontend/package-lock.json': '', | ||
| 'frontend/packages/ui/package.json': manifest, | ||
| 'frontend/packages/ui/package-lock.json': '', | ||
| }); | ||
| expect(projectForDependency(root, ['vitest'])).toBe(join(root, 'frontend')); | ||
| }); | ||
| it('names every candidate instead of choosing between independent projects', () => { | ||
| const root = fixture({ | ||
| 'frontend/package.json': manifest, | ||
| 'frontend/package-lock.json': '', | ||
| 'backend/package.json': manifest, | ||
| 'backend/pnpm-lock.yaml': '', | ||
| }); | ||
| expect(() => projectForDependency(root, ['vitest'])).toThrow( | ||
| 'vitest is declared in multiple independent projects: backend/package.json, frontend/package.json; run from the intended directory or pass --cwd <directory>', | ||
| ); | ||
| }); | ||
| it('ignores dependencies, build output, hidden, gitignored and symlinked directories, and manifests without a lockfile', () => { | ||
| const files: Record<string, string> = { | ||
| '.gitignore': 'ignored/\n', | ||
| 'apps/.gitignore': 'local/\n', | ||
| 'unlocked/package.json': manifest, | ||
| }; | ||
| for (const dir of [ | ||
| 'node_modules/dep', | ||
| 'dist/example', | ||
| '.cache/example', | ||
| 'ignored', | ||
| 'apps/local', | ||
| ]) { | ||
| files[`${dir}/package.json`] = manifest; | ||
| files[`${dir}/package-lock.json`] = ''; | ||
| } | ||
| const root = fixture(files); | ||
| const outside = fixture({ 'package.json': manifest, 'package-lock.json': '' }); | ||
| symlinkSync(outside, join(root, 'linked'), 'dir'); | ||
| expect(projectForDependency(root, ['vitest'])).toBe(root); | ||
| }); | ||
| it('accepts alternate package names and all direct dependency fields', () => { | ||
| for (const field of [ | ||
| 'dependencies', | ||
| 'devDependencies', | ||
| 'optionalDependencies', | ||
| 'peerDependencies', | ||
| ]) { | ||
| const root = fixture({ | ||
| 'frontend/package.json': JSON.stringify({ [field]: { 'router-dom': '^6' } }), | ||
| 'frontend/package-lock.json': '', | ||
| }); | ||
| expect(projectForDependency(root, ['router', 'router-dom'])).toBe(join(root, 'frontend')); | ||
| } | ||
| }); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| import { existsSync, readdirSync, readFileSync } from 'node:fs'; | ||
| import { join, relative, resolve } from 'node:path'; | ||
| import { UptideError } from './errors.js'; | ||
| import { defaultSkip, directoryRules, type IgnoreRule, ignoredBy } from './list/ignore.js'; | ||
| import { workspacePackagesOf } from './workspaces.js'; | ||
|
|
||
| /** An independent install boundary, not just another manifest in a workspace. */ | ||
| export function hasProjectLockfile(root: string): boolean { | ||
| return [ | ||
| 'pnpm-lock.yaml', | ||
| 'package-lock.json', | ||
| 'npm-shrinkwrap.json', | ||
| 'yarn.lock', | ||
| 'bun.lock', | ||
| 'bun.lockb', | ||
| ].some((file) => existsSync(join(root, file))); | ||
| } | ||
|
|
||
| /** A child lockfile is independent only when the ancestor does not declare it as a workspace. */ | ||
| export function isIndependentProject(root: string, ancestor: string): boolean { | ||
| return ( | ||
| hasProjectLockfile(root) && | ||
| !workspacePackagesOf(ancestor).some( | ||
| (workspace) => resolve(ancestor, workspace) === resolve(root), | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| function declares(root: string, names: string[]): boolean { | ||
| return workspacePackagesOf(root).some((workspace) => { | ||
| const file = join(root, workspace, 'package.json'); | ||
| if (!existsSync(file)) return false; | ||
| const pkg = JSON.parse(readFileSync(file, 'utf8')); | ||
| return names.some((name) => | ||
| ['dependencies', 'devDependencies', 'optionalDependencies', 'peerDependencies'].some( | ||
| (field) => Object.hasOwn(pkg[field] ?? {}, name), | ||
| ), | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Keep the requested project when it declares the dependency. Otherwise select the unique | ||
| * nested install root that does. Independent projects are never combined into a workspace: | ||
| * their package managers, lockfiles, configuration and verification remain separate. | ||
| */ | ||
| export function projectForDependency(cwd: string, names: string[]): string { | ||
| const root = resolve(cwd); | ||
| if (!existsSync(root) || declares(root, names)) return cwd; | ||
| const matches: string[] = []; | ||
| const walk = (dir: string, inherited: IgnoreRule[], members: Set<string>): void => { | ||
| const entries = readdirSync(dir, { withFileTypes: true }); | ||
| const rules = [ | ||
| ...inherited, | ||
| ...directoryRules( | ||
| dir, | ||
| entries.map((entry) => entry.name), | ||
| ), | ||
| ]; | ||
| for (const entry of entries) { | ||
| if ( | ||
| !entry.isDirectory() || | ||
| entry.name.startsWith('.') || | ||
| defaultSkip(entry.name) || | ||
| entry.name === 'out' | ||
| ) | ||
| continue; | ||
| const child = join(dir, entry.name); | ||
| if (ignoredBy(child, true, rules)) continue; | ||
| let nestedMembers = members; | ||
| if ( | ||
| !members.has(child) && | ||
| existsSync(join(child, 'package.json')) && | ||
| hasProjectLockfile(child) | ||
| ) { | ||
| if (declares(child, names)) matches.push(child); | ||
| nestedMembers = new Set([ | ||
| ...members, | ||
| ...workspacePackagesOf(child).map((w) => resolve(child, w)), | ||
| ]); | ||
| } | ||
| walk(child, rules, nestedMembers); | ||
| } | ||
| }; | ||
| walk(root, [], new Set(workspacePackagesOf(root).map((w) => resolve(root, w)))); | ||
| if (matches.length > 1) | ||
| throw new UptideError( | ||
| 'NO_PROJECT', | ||
| `${names.join(' or ')} is declared in multiple independent projects: ${matches | ||
| .map((dir) => `${relative(root, dir)}/package.json`) | ||
| .sort() | ||
| .join(', ')}; run from the intended directory or pass --cwd <directory>`, | ||
| ); | ||
| return matches[0] ?? cwd; | ||
| } | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 bug · medium
Broken nested manifests abort dependency discovery
The new discovery walk probes arbitrary directories, but parses them with strict, unguarded calls:
declaresrunsJSON.parseon every workspace member manifest with no error handling (packages/core/src/projects.ts:33), and the walk resolves nested workspaces with the strictworkspacePackagesOf, which throwsINVALID_WORKSPACEfor a brokenpnpm-workspace.yaml(packages/core/src/projects.ts:79). One malformed nestedpackage.json(or one whose JSON is not an object, which also throwsTypeErrorat the field access) or a broken workspace file therefore makesprojectForDependencythrow a rawSyntaxError,TypeError, orINVALID_WORKSPACEinstead of returning a project, failing the wholefixrun before any error reporting. The codebase already guards the root manifest indeclaredPatternsand exports the tolerantworkspacePackagesOrRootfor exactly these probe contexts, but this walk uses neither.📋 Prompt for AI Agents
In packages/core/src/projects.ts, harden the discovery probes: (1) in
declares(lines 29-40), wrapreadFileSync/JSON.parsein a try/catch that treats an unreadable or invalid manifest as not declaring the dependency, and guard that the parsed value is a non-null object before the field access; (2) replaceworkspacePackagesOfwithworkspacePackagesOrRootfrom './workspaces.js' for the directories probed during the walk (thedeclareshelper, theworkspacePackagesOf(child)call at line 79, and the seed call at line 85), and inisIndependentProject(lines 20-27), which is called fromprojectRoot(packages/core/src/fix/process.ts:127) and the ancestor loop in packages/cli/src/failures.ts:204. This prevents a malformed vendored/example package.json or a broken nested pnpm-workspace.yaml from abortinguptide fixwith a raw error before the intended NO_PROJECT/NOT_REPOSITORY_ROOT diagnostics.