Repository: oclif/core (ts)

Issue:
Bundled Oclif CLI with PKG is failing to initialize Package.json and Plugins
https://github.com/oclif/core/blob/a9c92867e7cafbff7f6e2339fe0aacb5c648203c/src/util/fs.ts#L13C11-L13C17

It seems it is not working as expected with a cli bundled with **PKG**.
When going through the Root Plugin initialization, and trying to read Package.json, the fs:promise `access` function throws with 
```bash
[Error: ENOENT: no such file or directory, access '/snapshot/mycli/package.json'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'access',
  path: '/snapshot/mycli/package.json'
}
```
I have debugged the content hosted by `pkg`:
```txt
------------------------------- virtual file system
/snapshot
  mycli                                63381336 
    run.js                                     533 
    package.json                              2356 
    oclif.manifest.json                       1965 
    dist                                     13275 
      index.js                                 238 
      commands                                7641 
        hello                                  7403 
          index.js                            1300 
      utils                                   8421 
        some                                    780 
          index.js                             780 
```
When replacing that line with using `readFile` or `open` (instead of `access`, as suggested by https://nodejs.org/api/fs.html#fsaccesspath-mode-callback), everything works as expected. 
Probably a permission error in the virtualized system?

Pull request:
fix: avoid fs.access for checking for file existence
No longer use `fs.access` to check for file existence, which is not recommended,

>Using fsPromises.access() to check for the accessibility of a file before calling fsPromises.open() is not recommended. Doing so introduces a race condition, since other processes may change the file's state between the two calls. Instead, user code should open/read/write the file directly and handle the error raised if the file is not accessible. 

Source: https://nodejs.org/api/fs.html#fspromisesaccesspath-mode

Fixes #815

@W-14266572@

Patch:
diff --git a/package.json b/package.json
index 2ea4969..9b34951 100644
--- a/package.json
+++ b/package.json
@@ -1,7 +1,7 @@
 {
   "name": "@oclif/core",
   "description": "base library for oclif CLIs",
-  "version": "3.0.4",
+  "version": "3.0.5-dev.0",
   "author": "Salesforce",
   "bugs": "https://github.com/oclif/core/issues",
   "dependencies": {
diff --git a/src/config/plugin.ts b/src/config/plugin.ts
index a63d0b8..340f42d 100644
--- a/src/config/plugin.ts
+++ b/src/config/plugin.ts
@@ -1,5 +1,6 @@
+/* eslint-disable no-await-in-loop */
 import {sync} from 'globby'
-import {dirname, join, parse, relative, sep} from 'node:path'
+import {basename, dirname, join, parse, relative, sep} from 'node:path'
 import {inspect} from 'node:util'
 
 import {Command} from '../command'
@@ -11,7 +12,7 @@ import {Topic} from '../interfaces/topic'
 import {loadWithData, loadWithDataFromManifest} from '../module-loader'
 import {OCLIF_MARKER_OWNER, Performance} from '../performance'
 import {cacheCommand} from '../util/cache-command'
-import {exists, readJson, requireJson} from '../util/fs'
+import {readJson, requireJson, safeReadJson} from '../util/fs'
 import {compact, isProd, mapValues} from '../util/util'
 import {tsPath} from './ts-node'
 import {Debug, getCommandIdPermutations, resolvePackage} from './util'
@@ -41,41 +42,56 @@ function* up(from: string) {
   yield from
 }
 
-async function findSourcesRoot(root: string) {
+async function findSourcesRoot(root: string, name?: string) {
+  // If we know the plugin name then we just need to traverse the file
+  // system until we find the directory that matches the plugin name.
+  if (name) {
+    for (const next of up(root)) {
+      if (next.endsWith(basename(name))) return next
+    }
+  }
+
+  // If there's no plugin name (typically just the root plugin), then we need
+  // to traverse the file system until we find a directory with a package.json
   for (const next of up(root)) {
-    const cur = join(next, 'package.json')
-    // eslint-disable-next-line no-await-in-loop
-    if (await exists(cur)) return dirname(cur)
+    // Skip the bin directory
+    if (
+      basename(dirname(next)) === 'bin' &&
+      ['dev', 'dev.cmd', 'dev.js', 'run', 'run.cmd', 'run.js'].includes(basename(next))
+    ) {
+      continue
+    }
+
+    try {
+      const cur = join(next, 'package.json')
+      if (await safeReadJson<PJSON>(cur)) return dirname(cur)
+    } catch {}
   }
 }
 
 /**
- * @returns string
- * @param name string
- * @param root string
- * find package root
- * for packages installed into node_modules this will go up directories until
- * it finds a node_modules directory with the plugin installed into it
+ * Find package root for packages installed into node_modules. This will go up directories
+ * until it finds a node_modules directory with the plugin installed into it
  *
  * This is needed because some oclif plugins do not declare the `main` field in their package.json
  * https://github.com/oclif/config/pull/289#issuecomment-983904051
+ *
+ * @returns string
+ * @param name string
+ * @param root string
  */
 async function findRootLegacy(name: string | undefined, root: string): Promise<string | undefined> {
   for (const next of up(root)) {
     let cur
     if (name) {
       cur = join(next, 'node_modules', name, 'package.json')
-      // eslint-disable-next-line no-await-in-loop
-      if (await exists(cur)) return dirname(cur)
-      try {
-        // eslint-disable-next-line no-await-in-loop
-        const pkg = await readJson<PJSON>(join(next, 'package.json'))
-        if (pkg.name === name) return next
-      } catch {}
+      if (await safeReadJson<PJSON>(cur)) return dirname(cur)
+
+      const pkg = await safeReadJson<PJSON>(join(next, 'package.json'))
+      if (pkg?.name === name) return next
     } else {
       cur = join(next, 'package.json')
-      // eslint-disable-next-line no-await-in-loop
-      if (await exists(cur)) return dirname(cur)
+      if (await safeReadJson<PJSON>(cur)) return dirname(cur)
     }
   }
 }
@@ -87,7 +103,7 @@ async function findRoot(name: string | undefined, root: string) {
       pkgPath = resolvePackage(name, {paths: [root]})
     } catch {}
 
-    return pkgPath ? findSourcesRoot(dirname(pkgPath)) : findRootLegacy(name, root)
+    return pkgPath ? findSourcesRoot(dirname(pkgPath), name) : findRootLegacy(name, root)
   }
 
   return findSourcesRoot(root)
diff --git a/src/util/fs.ts b/src/util/fs.ts
index ebf9dd9..ac6346a 100644
--- a/src/util/fs.ts
+++ b/src/util/fs.ts
@@ -1,5 +1,5 @@
-import {readFileSync} from 'node:fs'
-import {access, readFile, stat} from 'node:fs/promises'
+import {Stats, readFileSync} from 'node:fs'
+import {readFile, stat} from 'node:fs/promises'
 import {join} from 'node:path'
 
 const debug = require('debug')
@@ -8,34 +8,41 @@ export function requireJson<T>(...pathParts: string[]): T {
   return JSON.parse(readFileSync(join(...pathParts), 'utf8'))
 }
 
-export async function exists(path: string): Promise<boolean> {
+/**
+ * Parser for Args.directory and Flags.directory. Checks that the provided path
+ * exists and is a directory.
+ * @param input flag or arg input
+ * @returns Promise<string>
+ */
+export const dirExists = async (input: string): Promise<string> => {
+  let dirStat: Stats
   try {
-    await access(path)
-    return true
+    dirStat = await stat(input)
   } catch {
-    return false
-  }
-}
-
-export const dirExists = async (input: string): Promise<string> => {
-  if (!(await exists(input))) {
     throw new Error(`No directory found at ${input}`)
   }
 
-  const fileStat = await stat(input)
-  if (!fileStat.isDirectory()) {
+  if (!dirStat.isDirectory()) {
     throw new Error(`${input} exists but is not a directory`)
   }
 
   return input
 }
 
+/**
+ * Parser for Args.file and Flags.file. Checks that the provided path
+ * exists and is a file.
+ * @param input flag or arg input
+ * @returns Promise<string>
+ */
 export const fileExists = async (input: string): Promise<string> => {
-  if (!(await exists(input))) {
+  let fileStat: Stats
+  try {
+    fileStat = await stat(input)
+  } catch {
     throw new Error(`No file found at ${input}`)
   }
 
-  const fileStat = await stat(input)
   if (!fileStat.isFile()) {
     throw new Error(`${input} exists but is not a file`)
   }
@@ -55,3 +62,9 @@ export function readJsonSync<T = unknown>(path: string, parse = true): T | strin
   const contents = readFileSync(path, 'utf8')
   return parse ? (JSON.parse(contents) as T) : contents
 }
+
+export async function safeReadJson<T>(path: string): Promise<T | undefined> {
+  try {
+    return await readJson<T>(path)
+  } catch {}
+}


Test patch:
diff --git a/test/parser/parse.test.ts b/test/parser/parse.test.ts
index 7efd9e1..bb19aac 100644
--- a/test/parser/parse.test.ts
+++ b/test/parser/parse.test.ts
@@ -1552,11 +1552,9 @@ See more help with --help`)
 
   describe('fs flags', () => {
     const sandbox = createSandbox()
-    let accessStub: SinonStub
     let statStub: SinonStub
 
     beforeEach(() => {
-      accessStub = sandbox.stub(fs.promises, 'access')
       statStub = sandbox.stub(fs.promises, 'stat')
     })
 
@@ -1570,18 +1568,15 @@ See more help with --help`)
         const out = await parse([`--dir=${testDir}`], {
           flags: {dir: Flags.directory({exists: false})},
         })
-        expect(accessStub.callCount).to.equal(0)
         expect(out.flags).to.deep.include({dir: testDir})
       })
       it('passes if dir !exists but exists not defined', async () => {
         const out = await parse([`--dir=${testDir}`], {
           flags: {dir: Flags.directory()},
         })
-        expect(accessStub.callCount).to.equal(0)
         expect(out.flags).to.deep.include({dir: testDir})
       })
       it('passes when dir exists', async () => {
-        accessStub.resolves()
         statStub.returns({isDirectory: () => true})
         const out = await parse([`--dir=${testDir}`], {
           flags: {dir: Flags.directory({exists: true})},
@@ -1589,7 +1584,7 @@ See more help with --help`)
         expect(out.flags).to.deep.include({dir: testDir})
       })
       it("fails when dir doesn't exist", async () => {
-        accessStub.throws()
+        statStub.throws()
         try {
           const out = await parse([`--dir=${testDir}`], {
             flags: {dir: Flags.directory({exists: true})},
@@ -1601,7 +1596,6 @@ See more help with --help`)
         }
       })
       it('fails when dir exists but is not a dir', async () => {
-        accessStub.resolves()
         statStub.returns({isDirectory: () => false})
         try {
           const out = await parse([`--dir=${testDir}`], {
@@ -1616,7 +1610,6 @@ See more help with --help`)
       describe('custom parse functions', () => {
         const customParseException = 'NOT_OK'
         it('accepts custom parse that passes', async () => {
-          accessStub.resolves()
           statStub.returns({isDirectory: () => true})
           const out = await parse([`--dir=${testDir}`], {
             flags: {
@@ -1630,7 +1623,6 @@ See more help with --help`)
         })
 
         it('accepts custom parse that fails', async () => {
-          accessStub.resolves()
           statStub.returns({isDirectory: () => true})
           try {
             const out = await parse([`--dir=${testDir}`], {
@@ -1657,17 +1649,14 @@ See more help with --help`)
           flags: {file: Flags.file({exists: false})},
         })
         expect(out.flags).to.deep.include({file: testFile})
-        expect(accessStub.callCount).to.equal(0)
       })
       it("passes if file doesn't exist but not exists not defined", async () => {
         const out = await parse([`--file=${testFile}`], {
           flags: {file: Flags.file()},
         })
         expect(out.flags).to.deep.include({file: testFile})
-        expect(accessStub.callCount).to.equal(0)
       })
       it('passes when file exists', async () => {
-        accessStub.resolves()
         statStub.returns({isFile: () => true})
         const out = await parse([`--file=${testFile}`], {
           flags: {file: Flags.file({exists: true})},
@@ -1675,7 +1664,7 @@ See more help with --help`)
         expect(out.flags).to.deep.include({file: testFile})
       })
       it("fails when dir doesn't exist", async () => {
-        accessStub.throws()
+        statStub.throws()
         try {
           const out = await parse([`--file=${testFile}`], {
             flags: {file: Flags.file({exists: true})},
@@ -1687,7 +1676,6 @@ See more help with --help`)
         }
       })
       it('fails when file exists but is not a file', async () => {
-        accessStub.resolves()
         statStub.returns({isFile: () => false})
         try {
           const out = await parse([`--file=${testFile}`], {
@@ -1702,7 +1690,6 @@ See more help with --help`)
       describe('custom parse functions', () => {
         const customParseException = 'NOT_OK'
         it('accepts custom parse that passes', async () => {
-          accessStub.resolves()
           statStub.returns({isFile: () => true})
           const out = await parse([`--dir=${testFile}`], {
             flags: {
@@ -1716,7 +1703,6 @@ See more help with --help`)
         })
 
         it('accepts custom parse that fails', async () => {
-          accessStub.resolves()
           statStub.returns({isFile: () => true})
           try {
             const out = await parse([`--dir=${testFile}`], {
diff --git a/test/util/fs.test.ts b/test/util/fs.test.ts
index a8712aa..228e8be 100644
--- a/test/util/fs.test.ts
+++ b/test/util/fs.test.ts
@@ -1,6 +1,6 @@
 import {expect} from 'chai'
 
-import {readJson} from '../../src/util/fs'
+import {readJson, safeReadJson} from '../../src/util/fs'
 
 describe('readJson', () => {
   it('should return parsed JSON', async () => {
@@ -18,3 +18,15 @@ describe('readJson', () => {
     }
   })
 })
+
+describe('safeReadJson', () => {
+  it('should return parsed JSON', async () => {
+    const json = await safeReadJson<{name: string}>('package.json')
+    expect(json?.name).to.equal('@oclif/core')
+  })
+
+  it('should return undefined if the file does not exist', async () => {
+    const json = await safeReadJson('does-not-exist.json')
+    expect(json).to.be.undefined
+  })
+})