fix(cli): align ensureCheckout with PM-unified store layout

.claude/agent-memory/review-review-silent-failure-hunter/MEMORY.md

@@ -1,2 +1,3 @@
 - [Global store atomicity patterns](project_store_atomicity.md) — writeEntryAtomic/acquireEntryLock have known race windows; check before touching
 - [Shell pipe stderr loss pattern](feedback_shell_pipe_errors.md) — curl|tar execSync strings erase real errors; require -fsSL or execFileSync
+- [ensureCheckout fallback-ref path divergence](project_ensure_checkout_fallback_divergence.md) — PR #82 only fixed the naming axis; fetcher.fetch().storePath is still discarded, fallbackRefs/v-prefix winners silently exit 0 with no output

.claude/agent-memory/review-review-silent-failure-hunter/project_ensure_checkout_fallback_divergence.md

@@ -0,0 +1,11 @@
+---
+name: ensureCheckout fallback-ref path divergence
+description: ensureCheckout discards GithubSource.fetch().storePath; fallbackRefs/v-prefix winners silently produce wrong checkoutDir
+type: project
+---
+
+`packages/cli/src/commands/ensure-checkout.ts` computes `checkoutDir` from the *originally requested* `ref` and then discards the `FetchResult` from `fetcher.fetch(fetchOpts)`. But `GithubSource.fetch` may write to `<askHome>/github/github.com/<owner>/<repo>/<winningCandidate>/` where `winningCandidate` comes from `refCandidates(ref, fallbackRefs)` — i.e. a monorepo tag like `ai@6.0.159`, or the `v<ref>` variant produced inside `cloneAtTag`. The returned `storePath` reflects the winner; the discarded value is the one that matters.
+
+**Why:** PR #82 fixed the legacy-path variant of this bug (`githubCheckoutPath` → `githubStorePath`) but did not fix the fallback-ref variant. Downstream: `runDocs` → `findDocLikePaths(result.checkoutDir)` → `fs.existsSync` false → `[]` → exit 0 with no output. The source file even has a comment at lines 191-193 warning about this class of divergence, but only the naming axis is defended, not the ref-selection axis.
+
+**How to apply:** When reviewing `ensureCheckout` or any caller that trusts a pre-fetch-computed path, check whether the fetcher's actual output location is read back. Treat `await fetcher.fetch(...)` without using the return value as a silent-failure smell for any source that has fallback/retry ref logic. Also: any `runDocs`-style "emit N lines" loop should assert N > 0 before exit 0 — that invariant would catch every future variant of this class.

packages/cli/src/commands/ensure-checkout.ts

@@ -1,11 +1,13 @@
-import type { GithubSourceOptions, SourceConfig } from '../sources/index.js'
+import type { FetchResult, GithubSourceOptions, SourceConfig } from '../sources/index.js'
 import type { ParsedSpec } from '../spec.js'
 import fs from 'node:fs'
 import { npmEcosystemReader } from '../lockfiles/index.js'
 import { getResolver } from '../resolvers/index.js'
 import { GithubSource } from '../sources/github.js'
 import { parseSpec } from '../spec.js'
-import { githubCheckoutPath, resolveAskHome } from '../store/index.js'
+import { githubStorePath, resolveAskHome } from '../store/index.js'
+
+const DEFAULT_GITHUB_HOST = 'github.com'
 
 export interface EnsureCheckoutOptions {
   /** User-supplied spec, optionally with a trailing `@version` suffix. */
@@ -22,7 +24,7 @@ export interface EnsureCheckoutResult {
   repo: string
   ref: string
   resolvedVersion: string
-  /** Absolute path to `~/.ask/github/checkouts/<owner>__<repo>/<ref>/`. */
+  /** Absolute path to `~/.ask/github/<host>/<owner>/<repo>/<ref>/` (PM-unified store layout). */
   checkoutDir: string
   /**
    * For npm-ecosystem specs, the package name (e.g. `react`, `@vercel/ai`).
@@ -38,7 +40,7 @@ export interface EnsureCheckoutResult {
  */
 export interface EnsureCheckoutDeps {
   askHome?: string
-  fetcher?: { fetch: (opts: SourceConfig) => Promise<unknown> }
+  fetcher?: { fetch: (opts: SourceConfig) => Promise<FetchResult> }
   lockfileReader?: { read: (name: string, projectDir: string) => { version: string } | null }
   resolverFor?: (ecosystem: string) => {
     resolve: (name: string, version: string) => Promise<{
@@ -186,8 +188,10 @@ export async function ensureCheckout(
     fallbackRefs = result.fallbackRefs
   }
 
-  // 4. Compute the cache directory
-  const checkoutDir = githubCheckoutPath(askHome, owner, repo, ref)
+  // 4. Compute the cache directory — PM-unified layout, shared with
+  //    `GithubSource.fetch` which writes to the same path. If these two
+  //    ever diverge, `ask docs` / `ask src` silently emit no output.
+  const checkoutDir = githubStorePath(askHome, DEFAULT_GITHUB_HOST, owner, repo, ref)
 
   // 5. Cache hit short-circuit
   if (fs.existsSync(checkoutDir)) {
@@ -208,7 +212,16 @@ export async function ensureCheckout(
     ...(isFromBranch ? { branch: ref } : { tag: ref }),
     ...(fallbackRefs?.length ? { fallbackRefs } : {}),
   }
-  await fetcher.fetch(fetchOpts)
-
-  return { parsed, owner, repo, ref, resolvedVersion, checkoutDir, npmPackageName }
+  const fetchResult = await fetcher.fetch(fetchOpts)
+
+  // 8. Prefer the actual on-disk path reported by the fetcher. When a
+  //    `fallbackRef` wins or `cloneAtTag` rescues a non-`v` ref via
+  //    `v<ref>`, the store dir is under the WINNING candidate, not the
+  //    originally requested `ref`. Silently returning `checkoutDir`
+  //    (the primary-ref path) would reproduce the same empty-output bug
+  //    on the ref-candidate axis that the PM-unified layout fix closed
+  //    on the naming axis.
+  const resolvedCheckoutDir = fetchResult?.storePath ?? checkoutDir
+
+  return { parsed, owner, repo, ref, resolvedVersion, checkoutDir: resolvedCheckoutDir, npmPackageName }
 }

packages/cli/test/commands/cache-sharing.test.ts

@@ -4,20 +4,22 @@ import os from 'node:os'
 import path from 'node:path'
 import { afterEach, beforeEach, describe, expect, it, mock } from 'bun:test'
 import { ensureCheckout } from '../../src/commands/ensure-checkout.js'
-import { githubCheckoutPath } from '../../src/store/index.js'
+import { githubStorePath } from '../../src/store/index.js'
+
+const GH = 'github.com'
 
 /**
  * SC-6: After running `ask install` for a library that populates
- * `~/.ask/github/checkouts/<o>__<r>/<ref>/`, calling `ask src <spec>`
- * for the same library hits the EXACT same directory — zero
- * duplication.
+ * `~/.ask/github/<host>/<o>/<r>/<ref>/` (PM-unified layout), calling
+ * `ask src <spec>` for the same library hits the EXACT same directory
+ * — zero duplication.
  *
- * The eager (`install.ts` → `GithubSource.fetch`) and lazy
- * (`ensureCheckout` → `GithubSource.fetch`) paths must compute the
- * checkout path through the same `githubCheckoutPath(askHome, owner,
- * repo, ref)` helper. This test pins that contract by:
+ * The eager (`GithubSource.fetch`) and lazy (`ensureCheckout` →
+ * `GithubSource.fetch`) paths must compute the store path through the
+ * same `githubStorePath(askHome, 'github.com', owner, repo, ref)`
+ * helper. This test pins that contract by:
  *
- *   1. Computing the expected store path with `githubCheckoutPath`
+ *   1. Computing the expected store path with `githubStorePath`
  *      directly (the same way install.ts does it).
  *   2. Pre-populating that path with a fake checkout (simulating a
  *      prior `ask install` run).
@@ -44,9 +46,9 @@ afterEach(() => {
 })
 
 describe('cache sharing — install vs src/docs (SC-6)', () => {
-  it('ensureCheckout returns the same path that githubCheckoutPath computes', async () => {
+  it('ensureCheckout returns the same path that githubStorePath computes', async () => {
     // Pre-compute the path the eager pipeline would write to.
-    const expectedPath = githubCheckoutPath(askHome, 'facebook', 'react', 'v18.2.0')
+    const expectedPath = githubStorePath(askHome, GH, 'facebook', 'react', 'v18.2.0')
 
     // Simulate the directory left behind by a prior `ask install` run.
     fs.mkdirSync(expectedPath, { recursive: true })
@@ -87,7 +89,7 @@ describe('cache sharing — install vs src/docs (SC-6)', () => {
     // the helper hands the fetcher matches the path the eager pipeline
     // would use. The fake fetcher creates the dir to simulate a
     // successful fetch and we verify the post-condition.
-    const expectedPath = githubCheckoutPath(askHome, 'facebook', 'react', 'v18.2.0')
+    const expectedPath = githubStorePath(askHome, GH, 'facebook', 'react', 'v18.2.0')
 
     let fetchedRepo: string | undefined
     let fetchedTag: string | undefined
@@ -125,14 +127,13 @@ describe('cache sharing — install vs src/docs (SC-6)', () => {
     expect(result.checkoutDir).toBe(expectedPath)
   })
 
-  it('lazy and eager paths agree on the namespacing scheme (owner__repo / ref)', () => {
-    // This is a structural test: it asserts the directory layout is
-    // <askHome>/github/checkouts/<owner>__<repo>/<ref>/ — the layout
-    // both pipelines depend on. If anyone changes the separator or
-    // adds an intermediate segment, this test forces the rename to
-    // happen everywhere at once.
-    const p = githubCheckoutPath(askHome, 'facebook', 'react', 'v18.2.0')
-    const expected = path.join(askHome, 'github', 'checkouts', 'facebook__react', 'v18.2.0')
+  it('lazy and eager paths agree on the PM-unified layout (host/owner/repo/ref)', () => {
+    // Structural test: asserts the directory layout is
+    // <askHome>/github/<host>/<owner>/<repo>/<ref>/ — the PM-unified
+    // layout both pipelines depend on. If anyone changes the nesting
+    // scheme, this forces the rename to happen everywhere at once.
+    const p = githubStorePath(askHome, GH, 'facebook', 'react', 'v18.2.0')
+    const expected = path.join(askHome, 'github', GH, 'facebook', 'react', 'v18.2.0')
     expect(p).toBe(expected)
   })
 })

packages/cli/test/commands/ensure-checkout.integration.test.ts

@@ -0,0 +1,153 @@
+import type { GithubSourceOptions, SourceConfig } from '../../src/sources/index.js'
+import fs from 'node:fs'
+import os from 'node:os'
+import path from 'node:path'
+import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
+import { ensureCheckout } from '../../src/commands/ensure-checkout.js'
+import { runDocs } from '../../src/commands/docs.js'
+import { githubStorePath } from '../../src/store/index.js'
+
+/**
+ * Integration tests pinning the contract between `ensureCheckout` and the
+ * real `GithubSource.fetch` output layout.
+ *
+ * `GithubSource.fetch` writes to the PM-unified layout:
+ *   <askHome>/github/<host>/<owner>/<repo>/<tag>/
+ *
+ * `ensureCheckout`'s returned `checkoutDir` MUST match that exact layout —
+ * otherwise downstream callers like `ask docs` and `ask src` walk a path
+ * that does not exist and silently emit nothing (observed as "no output"
+ * exit 0 against real GitHub repos).
+ */
+
+let askHome: string
+let projectDir: string
+
+beforeEach(() => {
+  askHome = fs.mkdtempSync(path.join(os.tmpdir(), 'ask-ec-int-home-'))
+  projectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ask-ec-int-proj-'))
+})
+
+afterEach(() => {
+  fs.rmSync(askHome, { recursive: true, force: true })
+  fs.rmSync(projectDir, { recursive: true, force: true })
+})
+
+describe('ensureCheckout ↔ GithubSource.fetch layout contract', () => {
+  it('returns the PM-unified store path that GithubSource.fetch writes to', async () => {
+    // Simulate GithubSource.fetch by writing to the real PM-unified layout.
+    const fetcher = {
+      fetch: async (opts: SourceConfig) => {
+        const gh = opts as GithubSourceOptions
+        const [owner, repo] = gh.repo.split('/')
+        const ref = gh.tag ?? gh.branch!
+        const storeDir = githubStorePath(askHome, 'github.com', owner, repo, ref)
+        fs.mkdirSync(storeDir, { recursive: true })
+        fs.writeFileSync(path.join(storeDir, 'README.md'), '# test')
+        return { files: [], resolvedVersion: ref, storePath: storeDir }
+      },
+    }
+
+    const result = await ensureCheckout(
+      { spec: 'github:agentskills/agentskills', projectDir },
+      {
+        askHome,
+        fetcher,
+        resolverFor: () => null,
+        lockfileReader: { read: () => null },
+      },
+    )
+
+    const expectedDir = githubStorePath(askHome, 'github.com', 'agentskills', 'agentskills', 'main')
+    expect(result.checkoutDir).toBe(expectedDir)
+    expect(fs.existsSync(result.checkoutDir)).toBe(true)
+  })
+
+  it('returns the winning candidate path when fetch falls back from ref to a fallbackRef', async () => {
+    // Guards against a second silent-failure vector: when the resolver
+    // provides `fallbackRefs` (monorepo tags) or `cloneAtTag` rescues a
+    // non-`v` ref via `v<ref>`, `GithubSource.fetch` writes to the
+    // *winning* candidate's path — which may differ from the primary
+    // `ref`. `ensureCheckout` MUST surface that path (via
+    // `FetchResult.storePath`), otherwise `ask docs` / `ask src` walk a
+    // directory that does not exist and print nothing.
+    const winningRef = 'ai@6.0.158' // the fallback
+    const fetcher = {
+      fetch: async (opts: SourceConfig) => {
+        const gh = opts as GithubSourceOptions
+        const [owner, repo] = gh.repo.split('/')
+        const storeDir = githubStorePath(askHome, 'github.com', owner, repo, winningRef)
+        fs.mkdirSync(storeDir, { recursive: true })
+        fs.writeFileSync(path.join(storeDir, 'README.md'), '# test')
+        // The primary tag is NOT created — only the fallback wins.
+        return { files: [], resolvedVersion: winningRef, storePath: storeDir }
+      },
+    }
+    const resolver = {
+      resolve: async () => ({
+        repo: 'vercel/ai',
+        ref: 'ai@6.0.159',
+        resolvedVersion: '6.0.159',
+        fallbackRefs: ['ai@6.0.158'],
+      }),
+    }
+
+    const result = await ensureCheckout(
+      { spec: 'npm:@vercel/ai@6.0.159', projectDir },
+      {
+        askHome,
+        fetcher,
+        resolverFor: () => resolver,
+        lockfileReader: { read: () => null },
+      },
+    )
+
+    const expectedDir = githubStorePath(askHome, 'github.com', 'vercel', 'ai', winningRef)
+    expect(result.checkoutDir).toBe(expectedDir)
+    expect(fs.existsSync(result.checkoutDir)).toBe(true)
+  })
+
+  it('runDocs emits the PM-unified checkout path so output is not empty after fetch', async () => {
+    // End-to-end guard for the bug where `ask docs github:foo/bar` exited 0
+    // with no output: ensureCheckout returned a legacy path that did not
+    // exist, so findDocLikePaths returned [] and nothing was printed.
+    const fetcher = {
+      fetch: async (opts: SourceConfig) => {
+        const gh = opts as GithubSourceOptions
+        const [owner, repo] = gh.repo.split('/')
+        const ref = gh.tag ?? gh.branch!
+        const storeDir = githubStorePath(askHome, 'github.com', owner, repo, ref)
+        fs.mkdirSync(path.join(storeDir, 'docs'), { recursive: true })
+        return { files: [], resolvedVersion: ref, storePath: storeDir }
+      },
+    }
+
+    const stdout: string[] = []
+    const stderr: string[] = []
+    let exitCode: number | null = null
+
+    await runDocs(
+      { spec: 'github:agentskills/agentskills', projectDir },
+      {
+        ensureCheckout: opts =>
+          ensureCheckout(opts, {
+            askHome,
+            fetcher,
+            resolverFor: () => null,
+            lockfileReader: { read: () => null },
+          }),
+        log: (m: string) => stdout.push(m),
+        error: (m: string) => stderr.push(m),
+        exit: (c: number) => {
+          exitCode = c
+        },
+      },
+    )
+
+    const storeDir = githubStorePath(askHome, 'github.com', 'agentskills', 'agentskills', 'main')
+    expect(stdout).toContain(storeDir)
+    expect(stdout).toContain(path.join(storeDir, 'docs'))
+    expect(stderr).toEqual([])
+    expect(exitCode).toBeNull()
+  })
+})