From a2e2d51e95f6a449eda68cf16bec7c300af573a3 Mon Sep 17 00:00:00 2001 From: Minsu Lee Date: Fri, 17 Apr 2026 00:29:06 +0900 Subject: [PATCH] fix(cli): skip doc extraction in ensureCheckout and preserve symlinks in cpDirAtomic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes `ask src github:gitbutlerapp/gitbutler` failing with "No docs directory found" and a latent ENOENT on subsequent cache hits. Bug 1 — GithubSource.fetch always called extractDocsFromDir, which is correct for runInstall but wrong for ask src / ask docs (only the checkout path is needed). Repos without a conventional docs/ directory threw through both the shallow-clone and tar.gz paths. Added `skipDocExtraction?: boolean` to GithubSourceOptions; three call sites in github.ts now skip extraction when the flag is set, returning files: [] with the populated storePath. ensureCheckout sets skipDocExtraction: true; runInstall is untouched. Bug 2 — cpDirAtomic called fs.cpSync without verbatimSymlinks, so Node resolved relative symlinks against the tmp source dir, baking absolute paths pointing at the soon-deleted ask-gh-clone-* tmpdir into the store. Every subsequent verifyEntry -> hashDir -> readFileSync then hit ENOENT on the broken symlink. Gitbutler's claude.md/CLAUDE.md -> AGENTS.md relative symlinks triggered this. Added verbatimSymlinks: true to the fs.cpSync call. Regression tests added: - 3 new tests in github.test.ts under "skipDocExtraction — callers that only need the checkout path": success without docs dir, master fallback + no docs, guardrail that default behavior still throws. - 1 new test in ensure-checkout.test.ts verifying skipDocExtraction: true is forwarded to the fetcher. Two CLAUDE.md gotchas added to prevent regression of both fixes. --- CLAUDE.md | 2 + packages/cli/src/commands/ensure-checkout.ts | 6 ++ packages/cli/src/sources/github.ts | 12 ++- packages/cli/src/sources/index.ts | 10 ++ packages/cli/src/store/index.ts | 11 +- .../cli/test/commands/ensure-checkout.test.ts | 26 +++++ packages/cli/test/sources/github.test.ts | 102 ++++++++++++++++++ 7 files changed, 165 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 737ba4a..fa05b51 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -56,6 +56,8 @@ node packages/cli/dist/index.js docs add -s [options] - `parseSpec` lives in `packages/cli/src/spec.ts` and returns a discriminated union (`npm` / `github` / `unknown`). It is the single source of truth for translating an `ask.json` spec string into a library slug — do not reintroduce the legacy private `parseSpec(spec) -> { name, version }` from the old `index.ts`. - In Nuxt Content v3 server routes, `queryCollection` is auto-imported — do NOT use `import { queryCollection } from '#content/server'` (that alias does not exist and breaks Cloudflare Pages build). Use the bare function or `import { queryCollection } from '@nuxt/content/server'`. - Top-level commands: `ask install | add | remove | list | docs | skills | src | cache` (registered in `packages/cli/src/index.ts:323`). The **legacy** `ask docs` namespace that had `add | sync | list | remove` sub-commands is gone — there is no deprecated wrapper. The **current** `ask docs ` is a NEW single-shot command (`packages/cli/src/commands/docs.ts`) that prints candidate doc paths: for npm specs it walks `node_modules//`, then always walks the cached checkout at `/github/////` and emits the root plus any `/doc/i` subdirs. It shares `ensureCheckout` with `ask src` and does NOT go through `runInstall`, so strict ref validation does not apply — unqualified github specs default `ref = 'main'` and clone the branch directly. `ask install` is `postinstall`-friendly: per-entry failures emit a warning and exit code is always 0 (FR-10). +- `ensureCheckout` (shared by `ask src` and `ask docs`) always sets `skipDocExtraction: true` on its `GithubSourceOptions`. The source's `extractDocsFromDir` scan is a correctness precondition for `runInstall` (materializing `.ask/docs/@/` requires a real file list), but for `ask src`/`ask docs` it is actively wrong — those commands walk the checkout themselves (`findDocLikePaths`) and must not crash on repos without a conventional `docs/` folder (regression: `gitbutlerapp/gitbutler`). Keep the flag opt-in; never set it inside `runInstall`. +- `cpDirAtomic` (`packages/cli/src/store/index.ts`) passes `verbatimSymlinks: true` to `fs.cpSync`. Without it, Node resolves relative symlinks against the SOURCE directory at copy time, baking an absolute path pointing at the soon-deleted `ask-gh-clone-*` tmp dir into the store entry — every later `verifyEntry` call then hits ENOENT reading the now-broken link (observed on gitbutler's `claude.md`/`CLAUDE.md` → `AGENTS.md` links). `hashDir` walks every file via `collectFiles` + `readFileSync`, so it has no tolerance for a broken symlink. Keep the flag. - PM-driven entries (`{ "spec": "npm:" }`) get their version from the project's lockfile in priority order: `bun.lock → package-lock.json → pnpm-lock.yaml → yarn.lock → package.json` (range fallback). The chain lives in `packages/cli/src/lockfiles/index.ts:npmEcosystemReader`. Standalone github entries (`{ "spec": "github:owner/repo", "ref": "v1.2.3" }`) require an explicit `ref` enforced at the schema layer — there is no implicit default branch. - A PreToolUse Bash hook blocks shell commands when the current commit has no recorded review. Run `/review:code-review` (or `/review:run-cubic` / `/review:run-gemini`) and let it call `save-review-state.sh` before further bash. - `apps/registry/` uses `vercel.ts` (programmatic config via `@vercel/config` devDep), not `vercel.json`. Use `git.deploymentEnabled` (not deprecated `github.enabled`) to control auto-deploy. diff --git a/packages/cli/src/commands/ensure-checkout.ts b/packages/cli/src/commands/ensure-checkout.ts index a448ad3..6ffc1d5 100644 --- a/packages/cli/src/commands/ensure-checkout.ts +++ b/packages/cli/src/commands/ensure-checkout.ts @@ -216,6 +216,11 @@ export async function ensureCheckout( // For implicit default refs, pass NEITHER `tag` nor `branch` so // GithubSource.fetch can activate its default-branch fallback // chain (main → vmain → master). + // + // `skipDocExtraction: true` — callers of `ensureCheckout` only + // need the cached source tree on disk; they walk it themselves + // (`findDocLikePaths`) and must not fail when the repo has no + // conventional `docs/` folder (regression: gitbutlerapp/gitbutler). const refOpt: Partial = isImplicitDefaultRef ? {} : isFromBranch ? { branch: ref } : { tag: ref } @@ -224,6 +229,7 @@ export async function ensureCheckout( name: parsed.name, version: resolvedVersion, repo: `${owner}/${repo}`, + skipDocExtraction: true, ...refOpt, ...(fallbackRefs?.length ? { fallbackRefs } : {}), } diff --git a/packages/cli/src/sources/github.ts b/packages/cli/src/sources/github.ts index 7cd0f3c..a2f14b7 100644 --- a/packages/cli/src/sources/github.ts +++ b/packages/cli/src/sources/github.ts @@ -286,7 +286,9 @@ export class GithubSource implements DocSource { if (!fs.existsSync(storeDir)) continue if (verifyEntry(storeDir)) { - const files = this.extractDocsFromDir(storeDir, repo, ref, docsPath) + const files = opts.skipDocExtraction + ? [] + : this.extractDocsFromDir(storeDir, repo, ref, docsPath) return { files, resolvedVersion, @@ -359,7 +361,9 @@ export class GithubSource implements DocSource { } } - const files = this.extractDocsFromDir(storeDir, repo, ref, docsPath) + const files = opts.skipDocExtraction + ? [] + : this.extractDocsFromDir(storeDir, repo, ref, docsPath) return { files, resolvedVersion, @@ -446,7 +450,9 @@ export class GithubSource implements DocSource { } } - const files = this.extractDocsFromDir(storeDir, repo, candidate, docsPath) + const files = opts.skipDocExtraction + ? [] + : this.extractDocsFromDir(storeDir, repo, candidate, docsPath) return { files, resolvedVersion, diff --git a/packages/cli/src/sources/index.ts b/packages/cli/src/sources/index.ts index 097d15a..444e1cf 100644 --- a/packages/cli/src/sources/index.ts +++ b/packages/cli/src/sources/index.ts @@ -37,6 +37,16 @@ export interface GithubSourceOptions { * URL or path); production code should leave this unset. */ remoteUrl?: string + /** + * Skip the "walk the checkout for a docs directory" step after the + * clone lands in the store. Set by callers that only need the cached + * source tree on disk (e.g. `ask src`, `ask docs`) — those commands + * walk the directory themselves and must not fail when a repo has no + * conventional `docs/` folder (e.g. gitbutlerapp/gitbutler). The + * clone, verify, stamp, quarantine, and ref-fallback logic still run + * as before; only the final `extractDocsFromDir` call is bypassed. + */ + skipDocExtraction?: boolean } export interface WebSourceOptions { diff --git a/packages/cli/src/store/index.ts b/packages/cli/src/store/index.ts index 5e95fbe..a4a8d16 100644 --- a/packages/cli/src/store/index.ts +++ b/packages/cli/src/store/index.ts @@ -207,12 +207,21 @@ export function writeEntryAtomic( * Copies to a temp directory first, then renames. Mirrors the atomicity * guarantee of `writeEntryAtomic` for the case where source files are * already on disk (e.g., extracted GitHub tarballs). + * + * `verbatimSymlinks: true` preserves the exact symlink target written + * in the source tree. Without it, Node resolves relative symlinks + * against the SOURCE directory at copy time, baking absolute paths + * pointing at the (soon-deleted) `ask-gh-clone-*` tmp dir into the + * store — once the tmp dir is cleaned up, every later `verifyEntry` + * would hit ENOENT reading the broken symlink. Observed on + * `gitbutlerapp/gitbutler`, whose `claude.md`/`CLAUDE.md` are relative + * symlinks to `AGENTS.md`. */ export function cpDirAtomic(sourceDir: string, targetDir: string): void { const tmpDir = `${targetDir}.tmp-${crypto.randomUUID().slice(0, 8)}` fs.mkdirSync(path.dirname(tmpDir), { recursive: true }) try { - fs.cpSync(sourceDir, tmpDir, { recursive: true }) + fs.cpSync(sourceDir, tmpDir, { recursive: true, verbatimSymlinks: true }) if (fs.existsSync(targetDir)) { const backupDir = `${targetDir}.bak-${crypto.randomUUID().slice(0, 8)}` fs.renameSync(targetDir, backupDir) diff --git a/packages/cli/test/commands/ensure-checkout.test.ts b/packages/cli/test/commands/ensure-checkout.test.ts index a165a36..eddb698 100644 --- a/packages/cli/test/commands/ensure-checkout.test.ts +++ b/packages/cli/test/commands/ensure-checkout.test.ts @@ -286,6 +286,32 @@ describe('ensureCheckout', () => { expect(calls[0].opts.branch).toBeUndefined() }) + it('passes skipDocExtraction=true to fetcher so docs-less repos do not fail', async () => { + // Regression for: `bunx @pleaseai/ask src github:gitbutlerapp/gitbutler` + // failing with `No docs directory found` because `GithubSource.fetch` + // unconditionally walked the checkout for a `docs/` dir even though + // `ensureCheckout`'s callers (ask src, ask docs) only need the + // cached source tree. The flag tells the source to skip doc + // extraction after the clone lands in the store. + const { fetcher, calls } = makeFetcher(() => { + const dir = path.join(askHome, 'github', 'github.com', 'gitbutlerapp', 'gitbutler', 'main') + fs.mkdirSync(dir, { recursive: true }) + }) + + await ensureCheckout( + { spec: 'github:gitbutlerapp/gitbutler', projectDir }, + { + askHome, + fetcher, + resolverFor: () => null, + lockfileReader: { read: () => null }, + }, + ) + + expect(calls).toHaveLength(1) + expect(calls[0].opts.skipDocExtraction).toBe(true) + }) + it('handles github: spec with explicit @ref', async () => { const expectedDir = path.join(askHome, 'github', 'github.com', 'facebook', 'react', 'v18.2.0') diff --git a/packages/cli/test/sources/github.test.ts b/packages/cli/test/sources/github.test.ts index 02a6234..87efc10 100644 --- a/packages/cli/test/sources/github.test.ts +++ b/packages/cli/test/sources/github.test.ts @@ -455,6 +455,108 @@ describe('default-branch fallback — main → master', () => { }) }) +describe('skipDocExtraction — callers that only need the checkout path', () => { + /** + * Regression for `bunx @pleaseai/ask src github:gitbutlerapp/gitbutler` + * failing with `No docs directory found in gitbutlerapp/gitbutler@master`: + * + * `ask src` (and `ask docs`) go through `ensureCheckout`, which calls + * `GithubSource.fetch` to land the clone in the store. Before this + * flag, `fetch` unconditionally scanned the checkout for a `docs/` + * subdirectory and threw when none existed — even though `ask src` + * never needs the docs-file list, only the cached source tree on disk. + */ + function createNoDocsRemote(): string { + const repoDir = path.join(tmpDir, 'no-docs-remote.git') + const workDir = path.join(tmpDir, 'no-docs-work') + + fs.mkdirSync(workDir, { recursive: true }) + execFileSync('git', ['init', '-b', 'master', workDir], { stdio: 'ignore' }) + execFileSync('git', ['-C', workDir, 'config', 'user.email', 'test@test.com'], { stdio: 'ignore' }) + execFileSync('git', ['-C', workDir, 'config', 'user.name', 'Test'], { stdio: 'ignore' }) + + // Intentionally no docs/ — mirrors gitbutlerapp/gitbutler's layout. + fs.writeFileSync(path.join(workDir, 'README.md'), '# No Docs Here\n') + fs.writeFileSync(path.join(workDir, 'src.ts'), 'export {}\n') + + execFileSync('git', ['-C', workDir, 'add', '-A'], { stdio: 'ignore' }) + execFileSync('git', ['-C', workDir, 'commit', '-m', 'initial'], { stdio: 'ignore' }) + + execFileSync('git', ['clone', '--bare', workDir, repoDir], { stdio: 'ignore' }) + return repoDir + } + + it('succeeds with empty files and storePath when repo has no docs directory', async () => { + const remoteUrl = createNoDocsRemote() + const source = new GithubSource() + + const result = await source.fetch({ + source: 'github', + name: 'no-docs-repo', + version: 'master', + repo: 'test/no-docs-repo', + skipDocExtraction: true, + remoteUrl, + } as any) + + expect(result.files).toEqual([]) + expect(result.storePath).toBeDefined() + expect(fs.existsSync(result.storePath!)).toBe(true) + expect(fs.existsSync(path.join(result.storePath!, 'README.md'))).toBe(true) + }) + + it('still activates the main→master fallback when skipDocExtraction=true', async () => { + // Combines the gitbutler scenario: default branch is `master` AND + // there is no `docs/` dir. Both failure modes must be neutralized. + const remoteUrl = createNoDocsRemote() + const source = new GithubSource() + + const result = await source.fetch({ + source: 'github', + name: 'no-docs-repo', + version: 'main', + repo: 'test/no-docs-repo', + skipDocExtraction: true, + remoteUrl, + } as any) + + expect(result.storePath).toContain('master') + expect(result.files).toEqual([]) + }) + + it('default behavior (flag unset) still throws when no docs directory exists', async () => { + // Guardrail: `runInstall` still depends on the throw — it materializes + // `.ask/docs/@/` from `FetchResult.files`, so an empty + // list would silently publish an empty docs tree. The flag is opt-in. + // + // With the flag unset, the shallow-clone path's `extractDocsFromDir` + // rejects the no-docs repo. The outer `fetch()` catches and logs a + // warn, then falls through to the tar.gz path — which for this + // fake local remote 404s on github.com. Either way, an error + // escapes: the point is that `runInstall` does NOT receive a + // successful empty-files result. + const remoteUrl = createNoDocsRemote() + const source = new GithubSource() + + let capturedError: Error | null = null + try { + await source.fetch({ + source: 'github', + name: 'no-docs-repo', + version: 'master', + repo: 'test/no-docs-repo', + branch: 'master', + remoteUrl, + } as any) + } + catch (err) { + capturedError = err as Error + } + + expect(capturedError).not.toBeNull() + }) +}) + describe('refCandidates via fetch behavior — fallbackRefs', () => { /** * Create a local remote with monorepo-style tags (e.g. `ai@6.0.158`).