Merge branch 'task/command-feedback-master' into codex/otel-feedback-levels

scripts/project-doc-site.spec.ts

@@ -1,12 +1,14 @@
 /** Tests for the documentation website projection adapter. */
 
 import { execFileSync } from 'node:child_process'
-import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'
+import { existsSync, mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'
 import { tmpdir } from 'node:os'
 import { join, resolve } from 'node:path'
 import { afterEach, describe, expect, it } from 'vitest'
 import { docsPages, type DocsPage } from '../website/docs.ts'
-import { addProjectionFrontmatter, projectedPageContent, rewriteMarkdown } from './project-doc-site.ts'
+import {
+  addProjectionFrontmatter, projectedPageContent, publishableImage, rewriteMarkdown,
+} from './project-doc-site.ts'
 
 const roots: string[] = []
 const repositoryRoot = resolve(import.meta.dirname, '..')
@@ -63,6 +65,32 @@ describe('website source layout', () => {
   })
 })
 
+describe('publishableImage', () => {
+  it('accepts a regular file inside the repository', () => {
+    const { root } = fixture()
+    const real = realpathSync(join(root, 'packages/logo.svg'))
+    expect(publishableImage(join(root, 'packages/logo.svg'), realpathSync(root))).toBe(real)
+  })
+
+  it('refuses a target whose real path escapes the repository', () => {
+    // Publication copies the bytes onto the site, so a reference reaching a
+    // build-machine file must not be treated as an image the repository owns.
+    const { root } = fixture()
+    const outside = mkdtempSync(join(tmpdir(), 'dsh-doc-site-outside-'))
+    roots.push(outside)
+    writeFileSync(join(outside, 'secret.png'), 'not really a png\n')
+    symlinkSync(join(outside, 'secret.png'), join(root, 'packages/linked.png'))
+
+    expect(publishableImage(join(root, 'packages/linked.png'), realpathSync(root))).toBeUndefined()
+    expect(publishableImage(join(outside, 'secret.png'), realpathSync(root))).toBeUndefined()
+  })
+
+  it('refuses a directory', () => {
+    const { root } = fixture()
+    expect(publishableImage(join(root, 'packages'), realpathSync(root))).toBeUndefined()
+  })
+})
+
 describe('rewriteMarkdown', () => {
   it('maps published pages and pins unpublished source links', () => {
     const { root, pages } = fixture()
@@ -93,7 +121,7 @@ describe('rewriteMarkdown', () => {
     })).toBe('[B](./reference-root/b.md)\n')
   })
 
-  it('uses raw GitHub content for unpublished images', () => {
+  it('uses raw GitHub content for unpublished images when nothing places them', () => {
     const { root, pages } = fixture()
     expect(rewriteMarkdown('![logo](../packages/logo.svg)\n', {
       locale: 'en',
@@ -105,6 +133,57 @@ describe('rewriteMarkdown', () => {
     })).toBe('![logo](https://raw.githubusercontent.com/deepseek-harness/deepseek-harness/abc123/packages/logo.svg)\n')
   })
 
+  it('hands an image to the placer and uses the URL it returns', () => {
+    // A raw GitHub URL cannot serve a private repository, so the site build
+    // carries images itself; the placer is what puts them there. The stand-in
+    // derives its URL the way the real one does, so a placer that stopped
+    // returning the basename would fail here rather than pass on a constant.
+    const { root, pages } = fixture()
+    const placed: string[] = []
+    expect(rewriteMarkdown('![logo](../packages/logo.svg)\n', {
+      locale: 'en',
+      sourcePath: 'docs/a.md',
+      route: 'en/a.md',
+      pages,
+      repoRoot: root,
+      repositoryRef: 'abc123',
+      placeImage: (absPath) => {
+        const name = absPath.split('/').pop() ?? ''
+        placed.push(name)
+        return `./${name}`
+      },
+    })).toBe('![logo](./logo.svg)\n')
+    expect(placed).toEqual(['logo.svg'])
+  })
+
+  it('keeps a placed image\u2019s query or fragment', () => {
+    // An SVG view fragment and a Vite query both change what the reference
+    // means, and the GitHub branch has always carried them.
+    const { root, pages } = fixture()
+    expect(rewriteMarkdown('![logo](../packages/logo.svg#view)\n', {
+      locale: 'en',
+      sourcePath: 'docs/a.md',
+      route: 'en/a.md',
+      pages,
+      repoRoot: root,
+      repositoryRef: 'abc123',
+      placeImage: absPath => `./${absPath.split('/').pop() ?? ''}`,
+    })).toBe('![logo](./logo.svg#view)\n')
+  })
+
+  it('leaves a published page link to the route even when a placer exists', () => {
+    const { root, pages } = fixture()
+    expect(rewriteMarkdown('[B](b.md)\n', {
+      locale: 'en',
+      sourcePath: 'docs/a.md',
+      route: 'en/a.md',
+      pages,
+      repoRoot: root,
+      repositoryRef: 'abc123',
+      placeImage: () => { throw new Error('a page link must not be placed as an asset') },
+    })).toBe('[B](./reference/b.md)\n')
+  })
+
   it('does not rewrite Markdown-looking text inside code fences', () => {
     const { root, pages } = fixture()
     const source = '```md\n[B](b.md)\n```\n'

scripts/project-doc-site.ts

@@ -5,8 +5,10 @@
  * tier, while this adapter rewrites cross-source links for the public site.
  */
 
-import { existsSync, lstatSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
-import { dirname, extname, posix, relative, resolve, sep } from 'node:path'
+import {
+  copyFileSync, existsSync, lstatSync, mkdirSync, readFileSync, realpathSync, rmSync, statSync, writeFileSync,
+} from 'node:fs'
+import { basename, dirname, extname, posix, relative, resolve, sep } from 'node:path'
 import { fromMarkdown } from 'mdast-util-from-markdown'
 import { gfmFromMarkdown } from 'mdast-util-gfm'
 import { gfm } from 'micromark-extension-gfm'
@@ -38,6 +40,15 @@ export interface RewriteMarkdownOptions {
   pages: DocsPage[]
   repoRoot: string
   repositoryRef: string
+  /**
+   * Place one referenced image beside the projected page and return the URL to
+   * reach it from that page. A GitHub raw URL cannot serve this repository —
+   * `raw.githubusercontent.com` answers 404 for a private one, and no reader of
+   * the site is authenticated to it — so an image travels into the generated
+   * tree and Vite bundles it like any other site asset. Omitted by callers that
+   * only rewrite text, which then leave images pointing at the repository.
+   */
+  placeImage?: (absPath: string) => string
 }
 
 function repoPath(absPath: string, repoRoot: string): string {
@@ -222,9 +233,13 @@ export function rewriteMarkdown(source: string, options: RewriteMarkdownOptions)
       ? options.locale === 'root' ? 'en' : 'root'
       : options.locale
     const page = published.get(targetPath)?.get(targetLocale)
-    const nextUrl = page === undefined
-      ? githubTarget(absPath, line, suffix, options.repositoryRef, options.repoRoot, node.type === 'image')
-      : routeTarget(options.route, page.route, suffix)
+    const nextUrl = page !== undefined
+      ? routeTarget(options.route, page.route, suffix)
+      : node.type === 'image' && options.placeImage !== undefined
+        // The suffix rides along exactly as the GitHub branch keeps it: an SVG
+        // view fragment or a Vite query changes what the reference means.
+        ? `${options.placeImage(absPath)}${suffix}`
+        : githubTarget(absPath, line, suffix, options.repositoryRef, options.repoRoot, node.type === 'image')
 
     const start = node.position?.start.offset
     const end = node.position?.end.offset
@@ -291,17 +306,78 @@ export function projectedPageContent(markdown: string, page: DocsPage): string {
   return markdown.slice(0, closing + closingDelimiter.length)
 }
 
-/** Canonical Markdown files watched by the local VitePress dev server. */
+/**
+ * The repository file one image reference resolves to, or `undefined` when the
+ * target is not a local file this build may publish.
+ * @param absPath - resolved image target.
+ * @param repoRoot - repository root every published image must stay inside.
+ * @returns the file's real path, or `undefined` when it must not be copied.
+ *
+ * Only a regular file whose real path stays inside the repository qualifies.
+ * Publication copies the bytes into the site, so a reference escaping the
+ * repository — `../../.ssh/id_rsa`, or a symlink pointing out of the tree —
+ * would put a build-machine file on the site; `existsSync` alone, which is all
+ * link resolution needs, does not answer that.
+ */
+export function publishableImage(absPath: string, repoRoot: string): string | undefined {
+  const real = realpathSync(absPath)
+  const inside = real === repoRoot || real.startsWith(`${repoRoot}${sep}`)
+  return inside && statSync(real).isFile() ? real : undefined
+}
+
+/** Every local image a published page references, resolved to its repository file. */
+function referencedImages(): string[] {
+  const found = new Set<string>()
+  for (const page of docsPages) {
+    const sourceAbs = resolve(root, page.source)
+    if (!existsSync(sourceAbs)) continue
+    rewriteMarkdown(readFileSync(sourceAbs, 'utf8'), {
+      sourcePath: page.source,
+      locale: page.locale,
+      route: page.route,
+      pages: docsPages,
+      repoRoot: root,
+      repositoryRef: 'master',
+      placeImage: (absPath) => {
+        const real = publishableImage(absPath, root)
+        if (real !== undefined) found.add(real)
+        return ''
+      },
+    })
+  }
+  return [...found]
+}
+
+/**
+ * Files watched by the local VitePress dev server: every canonical Markdown
+ * source, plus the images they publish. Without the images, replacing a
+ * screenshot leaves the previous copy in the generated tree until something
+ * touches the Markdown beside it.
+ */
 export function docsSourceFiles(): string[] {
-  return [...new Set(docsPages.map(page => resolve(root, page.source)))]
+  return [...new Set([...docsPages.map(page => resolve(root, page.source)), ...referencedImages()])]
 }
 
 /** Rebuild the disposable VitePress source tree from the publication manifest. */
 export function projectDocs(): void {
   const routes = new Set<string>()
+  /** Projected path to the repository file that claimed it, pages and images alike. */
+  const claimed = new Map<string, string>()
   const repositoryRef = process.env.GITHUB_SHA ?? 'master'
   rmSync(generatedRoot, { recursive: true, force: true })
 
+  /** Reserve one projected path, refusing a second source for it. */
+  const claim = (target: string, sourceAbs: string): void => {
+    const holder = claimed.get(target)
+    if (holder !== undefined && holder !== sourceAbs) {
+      throw new Error(
+        `project-doc-site: ${repoPath(sourceAbs, root)} and ${repoPath(holder, root)}`
+        + ` both project to ${relative(generatedRoot, target).split(sep).join('/')}.`,
+      )
+    }
+    claimed.set(target, sourceAbs)
+  }
+
   for (const page of docsPages) {
     if (routes.has(page.route)) throw new Error(`project-doc-site: duplicate route ${JSON.stringify(page.route)}.`)
     routes.add(page.route)
@@ -310,6 +386,9 @@ export function projectDocs(): void {
       throw new Error(`project-doc-site: source ${JSON.stringify(page.source)} does not exist or is not a file.`)
     }
     const output = resolve(generatedRoot, page.route)
+    // Claimed before the images are placed: a page and an image landing on one
+    // path would otherwise overwrite each other in whichever order they ran.
+    claim(output, sourceAbs)
     mkdirSync(dirname(output), { recursive: true })
     const markdown = readFileSync(sourceAbs, 'utf8')
     const projected = rewriteMarkdown(markdown, {
@@ -319,6 +398,25 @@ export function projectDocs(): void {
       pages: docsPages,
       repoRoot: root,
       repositoryRef,
+      placeImage: (absPath) => {
+        const real = publishableImage(absPath, root)
+        if (real === undefined) {
+          throw new Error(
+            `project-doc-site: ${page.source} references image ${repoPath(absPath, root)},`
+            + ' which is not a regular file inside the repository.',
+          )
+        }
+        // Beside the page that references it, under its own basename: each
+        // locale's route tree gets its own copy, so one relative URL is correct
+        // from both.
+        const name = basename(real)
+        const target = resolve(dirname(output), name)
+        claim(target, real)
+        copyFileSync(real, target)
+        // Encoded because the destination is a Markdown inline target, where an
+        // unescaped space would end it early.
+        return `./${encodeURI(name)}`
+      },
     })
     writeFileSync(output, addProjectionFrontmatter(projectedPageContent(projected, page), page))
   }