diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index adc26f7..1a81a55 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -853,6 +853,12 @@ output: Atlassian Confluence publishing settings. +> **Not implemented.** RepoScribe has no Confluence renderer and no `docgen publish` command; the CLI has +> `init`, `generate`, `validate`, `diff` and `adr`. These settings are accepted by the configuration schema so +> that existing files still load, but setting `output.confluence.enabled: true` (or running +> `generate --format confluence`) makes every command that loads plugins stop with "Confluence output is not +> available". The `docgen publish` entries below describe intended behaviour, not a current command. + --- #### `output.confluence.enabled` @@ -863,8 +869,8 @@ Atlassian Confluence publishing settings. | **Type** | `boolean` | | **Required** | No | | **Default** | `false` | -| **Description** | When `true`, the `docgen publish` command pushes documentation pages to Confluence. | -| **CLI commands**| `docgen publish` | +| **Description** | Reserved. There is no Confluence renderer and no `docgen publish` command; `true` makes every command that loads plugins stop with an error (see the note above). | +| **CLI commands**| none | | **Related** | `output.confluence.baseUrl`, `output.confluence.spaceKey` | ```yaml diff --git a/docs/technical/cli.md b/docs/technical/cli.md index 7447d67..ae76e3c 100644 --- a/docs/technical/cli.md +++ b/docs/technical/cli.md @@ -54,7 +54,7 @@ src/ generate.ts # Documentation generation and watch mode validate.ts # Coverage validation with JSON output diff.ts # DocIR diff against a Git ref and breaking-change gate - adr.ts # Architecture Decision Records (114 lines) + adr.ts # Architecture Decision Records (124 lines) reporters/ index.ts # CI output formatters (82 lines) ``` @@ -210,12 +210,24 @@ async function generateCommand(options: GenerateOptions): Promise | Flag | Type | Default | Description | |-------------------------|----------|---------|----------------------------------------------------| -| `-f, --format ` | string[] | (all enabled) | Output format(s): `markdown`, `html`, `pdf`, `confluence` | +| `-f, --format ` | string[] | (all enabled) | Output format(s): `markdown`, `html`, `pdf`. `confluence` is accepted in the configuration but has no renderer and is rejected with a clear error | | `-o, --output ` | string | (from config) | Override output directory | | `--json` | boolean | `false` | Output result as JSON (for CI pipelines) | | `-v, --verbose` | boolean | `false` | Enable verbose logging | | `-w, --watch` | boolean | `false` | Watch for changes and regenerate | +#### Watch mode + +`--watch` generates once, then watches every configured language source root recursively +(`fs.watch`). Behaviour, covered by `src/commands/generate.watch.test.ts` and checked against the real +command: + +- A change starts a 200 ms timer; further changes restart it, so a burst of saves causes one regeneration. +- Only one generation runs at a time. A change that arrives while one is running is queued and runs after it. +- A failed generation is logged and watching continues. +- A watcher error ends the command with that error. +- `--watch` cannot be combined with `--json`. + #### Pipeline 1. **Load config:** `loadConfig(workDir)` reads `.docgen.yaml` from the current directory. @@ -421,7 +433,7 @@ Comparing documentation against HEAD~1... ### 4.5 `docgen adr [title]` -**Source:** `src/commands/adr.ts` (114 lines) +**Source:** `src/commands/adr.ts` (124 lines) **Signature:** @@ -462,7 +474,7 @@ Creates a new Architecture Decision Record file. **Validation:** If `title` is not provided, prints an error and exits with code 1. -**ID Assignment:** Reads all files in the ADR directory matching the pattern `/^ADR-\d+/`, counts them, and assigns the next sequential number. IDs are zero-padded to three digits: `ADR-001`, `ADR-002`, etc. +**ID Assignment:** Reads the names of the files in the ADR directory that match `/^ADR-(\d+)/` and assigns one more than the highest number found (`nextAdrNumber()`), so a gap or a deleted ADR never causes a number to be reused. Files that are not numbered ADRs, such as a `README.md` or `ADR-template.md`, are ignored. With none, the first ADR is `ADR-001`. IDs are zero-padded to three digits (`ADR-001`, `ADR-002`, ...) and grow past that without truncation (`ADR-1000`). **File Naming:** The file name is constructed as: diff --git a/docs/technical/core.md b/docs/technical/core.md index 60d51c7..dd18399 100644 --- a/docs/technical/core.md +++ b/docs/technical/core.md @@ -1816,6 +1816,8 @@ All configuration is defined in `.docgen.yaml` (or `.docgen.yml` or `docgen.conf #### `output.confluence` +> **Not implemented.** The settings below are accepted so existing configuration files still load, but RepoScribe has no Confluence renderer. Setting `enabled: true` makes plugin loading fail with a clear error. + | Option | Type | Default | Description | | ----------------- | ---------- | ---------------------- | -------------------------------------- | | `enabled` | `boolean` | `false` | Enable Confluence output | @@ -2277,7 +2279,7 @@ The `Orchestrator.loadAllPlugins()` method additionally: - `markdown` -> `@docgen/renderer-markdown` - `html` -> `@docgen/renderer-html` - `pdf` -> `@docgen/renderer-pdf` - - `confluence` -> `@docgen/renderer-confluence` + - `confluence` -> not available: there is no Confluence renderer, so `loadAllPlugins()` throws "Confluence output is not available: RepoScribe has no Confluence renderer." instead of trying to load a package - After loading external plugins, instantiates and registers the built-in `CoverageAnalyzer` and `LinkResolver` transformers - Sorts all transformers by priority (ascending) - Caches the `PluginRegistry` so subsequent calls return the same instance diff --git a/docs/technical/parser-java.md b/docs/technical/parser-java.md index ab4875a..130b58e 100644 --- a/docs/technical/parser-java.md +++ b/docs/technical/parser-java.md @@ -142,7 +142,7 @@ Other member fields: - `visibility`: `private`, `protected` or `public` from the modifiers; anything else (package-private, or an interface method with no modifier) is `internal`. - `isStatic` and `isAbstract` come from the modifier text; `isAsync` is always `false`. - `signature` for methods and constructors is the text up to the body. -- `parameters`: `name`, `type`, `isRest` (true for varargs); `isOptional` is always `false`. +- `parameters`: `name`, `type`, `description`, `isRest` (true for varargs); `isOptional` is always `false`. `description` is the text of the matching `@param` tag in the Javadoc of the method or constructor, or an empty string. A varargs parameter (`String... parts`) is reported under its own name with the array type of its element (`String[]`). A `@param` tag that names no real parameter is ignored there but stays in the member's `tags`. Covered by `src/index.test.ts`. - `deprecated`: set only from a Javadoc `@deprecated` tag. The `@Deprecated` annotation does not set it. ### Javadoc handling @@ -217,7 +217,7 @@ Example, taken from the package test (`UserController.getUser`): - `DecoratorNode.arguments` is always `{}`; only the raw annotation text is kept. - Interface `extends` lists are not recorded as `implements` (the parser reads the grammar's `interfaces` field only). - `isNullable` and `isUnion` on type references are always `false`; arrays are recognised only by a trailing `[]`. -- **Parameter descriptions are always empty.** The description lookup reads a Javadoc comment before the parameter list rather than before the method, so no comment is found. `@param` text is still available in the member's `tags`. No test covers this. +- *Fixed by AUDIT-001 (RS-05):* parameter descriptions used to be empty because the lookup read a comment before the parameter list instead of before the method, and a varargs parameter was reported as `arg` with type `Object`. Both are now tested. --- diff --git a/docs/technical/parser-typescript.md b/docs/technical/parser-typescript.md index e86c80e..b486388 100644 --- a/docs/technical/parser-typescript.md +++ b/docs/technical/parser-typescript.md @@ -158,6 +158,20 @@ matched by `input.include` / `input.exclude` globs rooted at --- +### Module cache + +One `TypeScriptParser` instance keeps the modules it extracted for each file, so a later `parse` call (watch +mode reuses one instance) skips files that have not changed. Covered by `src/cache.test.ts`. + +- A file is served from the cache when its modification time **and** size are both unchanged. A change to + either one parses the file again. +- Files that are no longer in the list passed to `parse` are dropped from the cache. +- Callers receive copies of the cached modules, so changing a result does not change the cache. +- The check does not read file contents: an edit that keeps both the size and the modification time is not + noticed. + +--- + ### File-Level Parsing #### `parseSourceFile(sourceFile: SourceFile, sourceRoot: string): ModuleNode[]` @@ -656,16 +670,18 @@ text.replace(/import\([^)]+\)\./g, "") --- -#### `extractParameters(params: ParameterDeclaration[]): ParamNode[]` +#### `extractParameters(params: ParameterDeclaration[], docNode: Node): ParamNode[]` Maps an array of ts-morph `ParameterDeclaration` nodes to DocIR `ParamNode` -objects. +objects. `docNode` is the node that carries the JSDoc block: the function, +method, constructor or accessor itself, or the variable statement for an arrow +function assigned to a `const`. ```typescript { name: p.getName(), type: this.buildTypeRef(p.getType()), - description: this.getParamDescription(p), + description: this.getParamDescription(p, docNode), isOptional: p.isOptional(), isRest: p.isRestParameter(), defaultValue: p.getInitializer()?.getText(), @@ -674,17 +690,18 @@ objects. --- -#### `getParamDescription(param: ParameterDeclaration): string` - -Walks up to the parameter's parent function/method, extracts all `@param` -tags, and finds the one whose `value` starts with the parameter name. Then -strips the parameter name and any leading `- ` separator from the value. +#### `getParamDescription(param: ParameterDeclaration, docNode: Node): string` -```typescript -paramTag.value.replace(new RegExp(`^${param.getName()}\\s*-?\\s*`), "") -``` +Looks through the JSDoc blocks on `docNode` for the `@param` tag whose name is +exactly the parameter's name (so `id` never picks up the text written for +`idx`). The text is the tag's comment: the optional `{type}`, the name and a +leading `-` are not part of it, and line breaks inside a multi-line description +become single spaces. -Returns `""` if no matching `@param` tag is found. +Returns `""` if no matching `@param` tag exists. Both `@param a - text` and +`@param a text` are accepted. Before this was corrected, `@param` text never +reached `ParamNode.description`, so documented parameters were reported as +undocumented by `docgen validate`. --- diff --git a/packages/cli/src/commands/adr.test.ts b/packages/cli/src/commands/adr.test.ts new file mode 100644 index 0000000..707e46d --- /dev/null +++ b/packages/cli/src/commands/adr.test.ts @@ -0,0 +1,196 @@ +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { adrCommand } from "./adr"; + +// AUDIT-001 / RS-14: the `adr` command had no tests. These pin what it does today. + +describe("adrCommand", () => { + let dir: string; + let log: ReturnType; + let error: ReturnType; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "reposcribe-adr-")); + vi.spyOn(process, "cwd").mockReturnValue(dir); + log = vi.spyOn(console, "log").mockImplementation(() => undefined); + error = vi.spyOn(console, "error").mockImplementation(() => undefined); + vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`process.exit(${code})`); + }) as never); + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-03-04T10:00:00Z")); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + const decisions = () => path.join(dir, "docs", "decisions"); + const output = (spy: ReturnType) => spy.mock.calls.map((call) => call.join(" ")).join("\n"); + + describe("new", () => { + it("creates ADR-001 in docs/decisions with the title, status and today's date", async () => { + await adrCommand("new", "Use PostgreSQL", { status: "proposed" }); + + const files = fs.readdirSync(decisions()); + expect(files).toHaveLength(1); + expect(files[0]).toMatch(/^ADR-001-use-postgresql/); + const content = fs.readFileSync(path.join(decisions(), files[0]), "utf-8"); + expect(content).toContain("# ADR-001: Use PostgreSQL"); + expect(content).toMatch(/## Status\s+proposed/); + expect(content).toMatch(/## Date\s+2026-03-04/); + for (const heading of ["## Context", "## Decision", "## Consequences", "## Related"]) { + expect(content).toContain(heading); + } + expect(output(log)).toContain("Created:"); + }); + + it("uses the status it is given", async () => { + await adrCommand("new", "Adopt React", { status: "accepted" }); + + const [file] = fs.readdirSync(decisions()); + expect(fs.readFileSync(path.join(decisions(), file), "utf-8")).toMatch(/## Status\s+accepted/); + }); + + it("numbers the next ADR after the existing ones and builds a lowercase hyphenated file name", async () => { + await adrCommand("new", "First", { status: "proposed" }); + await adrCommand("new", "Use PostgreSQL for data", { status: "proposed" }); + + const files = fs.readdirSync(decisions()).sort(); + expect(files[1]).toMatch(/^ADR-002-use-postgresql-for-data/); + }); + + it("refuses to create an ADR without a title", async () => { + await expect(adrCommand("new", undefined, { status: "proposed" })).rejects.toThrow("process.exit(1)"); + + expect(output(error)).toContain("Title is required"); + expect(fs.existsSync(decisions())).toBe(false); + }); + + it("writes to adr.directory from .docgen.yaml when one is configured", async () => { + fs.writeFileSync( + path.join(dir, ".docgen.yaml"), + [ + "project:", + " name: Demo", + "languages:", + " - name: typescript", + " source: src", + ' parser: "@docgen/parser-typescript"', + "adr:", + " directory: records/adr", + "", + ].join("\n"), + ); + + await adrCommand("new", "Configured location", { status: "proposed" }); + + expect(fs.readdirSync(path.join(dir, "records", "adr"))[0]).toMatch(/^ADR-001-configured-location/); + expect(fs.existsSync(decisions())).toBe(false); + }); + }); + + describe("list", () => { + it("explains how to create the first ADR when none exist", async () => { + await adrCommand("list", undefined, { status: "proposed" }); + + expect(output(log)).toContain("No ADRs found. Use 'docgen adr new ' to create one."); + }); + + it("lists each ADR with its status and title, sorted by file name, ignoring non-markdown files", async () => { + await adrCommand("new", "Alpha", { status: "accepted" }); + await adrCommand("new", "Beta", { status: "proposed" }); + fs.writeFileSync(path.join(decisions(), "notes.txt"), "not an ADR"); + + await adrCommand("list", undefined, { status: "proposed" }); + + const text = output(log); + expect(text).toContain("Architecture Decision Records (2):"); + expect(text.indexOf("ADR-001: Alpha")).toBeGreaterThan(-1); + expect(text.indexOf("ADR-001: Alpha")).toBeLessThan(text.indexOf("ADR-002: Beta")); + expect(text).toMatch(/accepted\s+# ADR-001: Alpha|accepted\s+ADR-001: Alpha/); + expect(text).toMatch(/proposed\s+ADR-002: Beta/); + }); + }); + + describe("unsupported actions", () => { + it("rejects `update`, which only the old help text advertised", async () => { + await expect(adrCommand("update", undefined, { status: "proposed" })).rejects.toThrow("process.exit(1)"); + + expect(output(error)).toContain("Unknown ADR action: update"); + expect(output(log)).toContain("Available actions: new, list"); + }); + }); + + // The audit found that numbering used the file count, so gaps produced duplicate numbers. + describe("numbering with gaps and other files", () => { + const seed = (...names: string[]) => { + fs.mkdirSync(decisions(), { recursive: true }); + for (const name of names) fs.writeFileSync(path.join(decisions(), name), `# ${name}\n`); + }; + + it("numbers after the highest existing number, not after the file count", async () => { + seed("ADR-001-first.md", "ADR-003-third.md"); + + await adrCommand("new", "Fourth", { status: "proposed" }); + + expect(fs.readdirSync(decisions()).sort()).toEqual([ + "ADR-001-first.md", + "ADR-003-third.md", + "ADR-004-fourth.md", + ]); + }); + + it("does not reuse a number after an earlier ADR was deleted", async () => { + await adrCommand("new", "One", { status: "proposed" }); + await adrCommand("new", "Two", { status: "proposed" }); + fs.rmSync(path.join(decisions(), fs.readdirSync(decisions()).sort()[0])); + + await adrCommand("new", "Three", { status: "proposed" }); + + expect(fs.readdirSync(decisions()).sort()).toEqual([ + expect.stringMatching(/^ADR-002-two/), + expect.stringMatching(/^ADR-003-three/), + ]); + }); + + it("keeps counting past three digits instead of truncating or repeating", async () => { + seed("ADR-999-nine-nine-nine.md", "ADR-1000-one-thousand.md"); + + await adrCommand("new", "Next", { status: "proposed" }); + + expect(fs.readdirSync(decisions())).toContain("ADR-1001-next.md"); + }); + + it("ignores files that are not numbered ADRs when choosing the number", async () => { + seed("README.md", "ADR-template.md", "notes-2024.md", "ADR-002-real.md"); + + await adrCommand("new", "After real", { status: "proposed" }); + + expect(fs.readdirSync(decisions())).toContain("ADR-003-after-real.md"); + expect(fs.readdirSync(decisions())).toHaveLength(5); + }); + + it("starts at ADR-001 when the folder holds no numbered ADRs", async () => { + seed("README.md"); + + await adrCommand("new", "First", { status: "proposed" }); + + expect(fs.readdirSync(decisions())).toContain("ADR-001-first.md"); + }); + + it("never overwrites an existing file", async () => { + seed("ADR-001-first.md"); + const before = fs.readFileSync(path.join(decisions(), "ADR-001-first.md"), "utf-8"); + + await adrCommand("new", "First", { status: "proposed" }); + + expect(fs.readFileSync(path.join(decisions(), "ADR-001-first.md"), "utf-8")).toBe(before); + expect(fs.readdirSync(decisions())).toContain("ADR-002-first.md"); + }); + }); +}); diff --git a/packages/cli/src/commands/adr.ts b/packages/cli/src/commands/adr.ts index 79cb989..1608b81 100644 --- a/packages/cli/src/commands/adr.ts +++ b/packages/cli/src/commands/adr.ts @@ -33,6 +33,23 @@ export async function adrCommand( } } +/** + * Choose the number for a new ADR: one more than the highest `ADR-<n>` file name. + * Counting files would reuse a number after a deletion or a gap, and files that + * are not numbered ADRs (a README, a template) must not affect the result. + * + * @param fileNames - Names of the files in the ADR directory. + * @returns The number to give the next ADR; 1 when there are no numbered ADRs. + */ +export function nextAdrNumber(fileNames: string[]): number { + let highest = 0; + for (const name of fileNames) { + const match = /^ADR-(\d+)/.exec(name); + if (match) highest = Math.max(highest, Number(match[1])); + } + return highest + 1; +} + function createAdr( adrDir: string, title: string | undefined, @@ -47,10 +64,7 @@ function createAdr( fs.mkdirSync(adrDir, { recursive: true }); } - // Determine next ADR number - const existing = fs.readdirSync(adrDir).filter((f) => f.match(/^ADR-\d+/)); - const nextNum = existing.length + 1; - const id = `ADR-${String(nextNum).padStart(3, "0")}`; + const id = `ADR-${String(nextAdrNumber(fs.readdirSync(adrDir))).padStart(3, "0")}`; const fileName = `${id}-${title.toLowerCase().replace(/[^a-z0-9]+/g, "-")}.md`; const filePath = path.join(adrDir, fileName); diff --git a/packages/cli/src/commands/diff.exit-code.test.ts b/packages/cli/src/commands/diff.exit-code.test.ts new file mode 100644 index 0000000..230c90c --- /dev/null +++ b/packages/cli/src/commands/diff.exit-code.test.ts @@ -0,0 +1,191 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createEmptyCoverage, createEmptyDocIR, type DocIR, type ModuleNode } from "@docgen/core"; + +// AUDIT-001 / RS-09: `repo-scribe diff --fail-on-breaking` is the CI gate for API compatibility, so its +// exit code must be exactly: 1 when the change is breaking and the flag is set, otherwise unchanged. +// Git, tar, the filesystem and the documentation snapshots are replaced so only the command's own +// decisions are under test; `buildDiffReport` itself is covered in diff.test.ts. + +const mocks = vi.hoisted(() => ({ + execFile: vi.fn(), + mkdtemp: vi.fn(), + mkdir: vi.fn(), + rm: vi.fn(), + symlink: vi.fn(), + snapshots: new Map<string, unknown>(), + snapshotted: [] as string[], +})); + +vi.mock("node:child_process", () => ({ + execFile: (...args: unknown[]) => mocks.execFile(...args), +})); + +vi.mock("node:fs/promises", () => ({ + mkdir: (...args: unknown[]) => mocks.mkdir(...args), + mkdtemp: (...args: unknown[]) => mocks.mkdtemp(...args), + rm: (...args: unknown[]) => mocks.rm(...args), + symlink: (...args: unknown[]) => mocks.symlink(...args), +})); + +vi.mock("@docgen/core", async (importOriginal) => ({ + ...(await importOriginal<typeof import("@docgen/core")>()), + loadConfig: () => ({}), + Orchestrator: class { + constructor(private readonly options: { workDir: string }) {} + + async snapshot() { + mocks.snapshotted.push(this.options.workDir); + return mocks.snapshots.get(this.options.workDir === process.cwd() ? "current" : "base"); + } + }, +})); + +import { diffCommand } from "./diff"; + +const TEMP_ROOT = "/tmp/docgen-diff-test"; + +describe("diffCommand exit code", () => { + let logSpy: ReturnType<typeof vi.spyOn>; + let errorSpy: ReturnType<typeof vi.spyOn>; + let savedExitCode: typeof process.exitCode; + let failingCommand: string | undefined; + + beforeEach(() => { + savedExitCode = process.exitCode; + process.exitCode = undefined; + failingCommand = undefined; + mocks.snapshotted.length = 0; + mocks.mkdtemp.mockReset().mockResolvedValue(TEMP_ROOT); + mocks.mkdir.mockReset().mockResolvedValue(undefined); + mocks.rm.mockReset().mockResolvedValue(undefined); + mocks.symlink.mockReset().mockResolvedValue(undefined); + mocks.execFile.mockReset().mockImplementation( + (file: string, args: string[], _options: unknown, callback: (error: Error | null, result?: unknown) => void) => { + const command = file === "git" ? `git ${args[0]}` : file; + if (command === failingCommand) { + callback(new Error(`${command} failed`)); + return; + } + const stdout = file === "git" && args[0] === "rev-parse" && args[1] === "HEAD" ? "abc123\n" : ""; + callback(null, { stdout, stderr: "" }); + } + ); + logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + }); + + afterEach(() => { + process.exitCode = savedExitCode; + logSpy.mockRestore(); + errorSpy.mockRestore(); + }); + + /** The base ref has module "Api"; the current tree does not, so the module removal is breaking. */ + function breakingChange(): void { + mocks.snapshots.set("base", documentWith([moduleNode("api", "Api")])); + mocks.snapshots.set("current", documentWith([])); + } + + function noChange(): void { + mocks.snapshots.set("base", documentWith([moduleNode("api", "Api")])); + mocks.snapshots.set("current", documentWith([moduleNode("api", "Api")])); + } + + const printedReport = (): { breaking: boolean; base: string; current: string } => + JSON.parse(String(logSpy.mock.calls.at(-1)?.[0])); + + it("exits 1 when the change is breaking and --fail-on-breaking is set", async () => { + breakingChange(); + await diffCommand({ base: "main", json: true, failOnBreaking: true }); + + expect(process.exitCode).toBe(1); + expect(printedReport()).toMatchObject({ breaking: true, base: "main", current: "abc123" }); + }); + + it("still reports a breaking change but leaves the exit code alone without the flag", async () => { + breakingChange(); + await diffCommand({ base: "main", json: true }); + + expect(process.exitCode).toBeUndefined(); + expect(printedReport().breaking).toBe(true); + }); + + it("does not fail a non-breaking change even when --fail-on-breaking is set", async () => { + noChange(); + await diffCommand({ base: "main", json: true, failOnBreaking: true }); + + expect(process.exitCode).toBeUndefined(); + expect(printedReport().breaking).toBe(false); + }); + + it("applies the same exit code in human-readable output", async () => { + breakingChange(); + await diffCommand({ base: "main", failOnBreaking: true }); + + expect(process.exitCode).toBe(1); + expect(logSpy).toHaveBeenCalled(); + }); + + it("compares the working tree with an archive of the requested ref, not by switching branches", async () => { + noChange(); + await diffCommand({ base: "release/1.x", json: true }); + + const gitCalls = mocks.execFile.mock.calls.filter(([file]) => file === "git").map(([, args]) => args as string[]); + const archive = gitCalls.find((args) => args[0] === "archive"); + expect(archive).toContain("release/1.x"); + expect(gitCalls.some((args) => ["checkout", "switch", "reset", "stash"].includes(args[0]))).toBe(false); + expect(mocks.snapshotted).toHaveLength(2); + }); + + it("exits 1 with a clear message and takes no snapshots when git fails", async () => { + breakingChange(); + failingCommand = "git rev-parse"; + await diffCommand({ base: "main", json: true }); + + expect(process.exitCode).toBe(1); + expect(errorSpy).toHaveBeenCalledWith("RepoScribe diff failed: git rev-parse failed"); + expect(mocks.snapshotted).toHaveLength(0); + expect(logSpy).not.toHaveBeenCalled(); + }); + + it("exits 1 when the base ref cannot be archived, and still removes the temporary directory", async () => { + breakingChange(); + failingCommand = "git archive"; + await diffCommand({ base: "no-such-ref", json: true }); + + expect(process.exitCode).toBe(1); + expect(errorSpy).toHaveBeenCalledWith("RepoScribe diff failed: git archive failed"); + expect(mocks.rm).toHaveBeenCalledWith(TEMP_ROOT, { recursive: true, force: true }); + }); + + it("removes the temporary directory after a successful run", async () => { + noChange(); + await diffCommand({ base: "main", json: true }); + + expect(mocks.rm).toHaveBeenCalledTimes(1); + expect(mocks.rm).toHaveBeenCalledWith(TEMP_ROOT, { recursive: true, force: true }); + }); +}); + +function documentWith(modules: ModuleNode[]): DocIR { + return { ...createEmptyDocIR({ name: "fixture" }), modules }; +} + +function moduleNode(id: string, name: string): ModuleNode { + return { + id, + name, + filePath: `src/${id}.ts`, + language: "typescript", + kind: "module", + description: `${name} module`, + tags: [], + members: [], + dependencies: [], + examples: [], + coverage: { ...createEmptyCoverage(), overall: 100 }, + decorators: [], + typeParameters: [], + sourceFacts: {} as ModuleNode["sourceFacts"], + }; +} diff --git a/packages/cli/src/commands/generate.ts b/packages/cli/src/commands/generate.ts index d5b9a1c..b6f1577 100644 --- a/packages/cli/src/commands/generate.ts +++ b/packages/cli/src/commands/generate.ts @@ -110,19 +110,44 @@ function writeArtifacts(workDir: string, config: any, result: GenerateResult): v } } -async function watchAndRegenerate( +/** Quiet period after the last file change before regenerating. */ +const WATCH_DEBOUNCE_MS = 200; + +/** Injection points so the watch loop can be tested without real file watchers or real time. */ +export interface WatchDependencies { + /** File watcher to use instead of `fs.watch`. */ + watch?: typeof fs.watch; + /** Quiet period in milliseconds after the last change before regenerating. */ + debounceMs?: number; +} + +/** + * Watch every configured source root and regenerate after changes settle. + * Changes that arrive during a generation are coalesced into one follow-up run. + * + * @param config - Loaded RepoScribe configuration; its language source roots are watched. + * @param workDir - Project directory that the source roots are resolved against. + * @param logger - Receives progress and generation errors. + * @param generate - Runs one generation; a rejection is logged and watching continues. + * @param dependencies - Optional watcher and debounce overrides, used by tests. + * @returns A promise that never resolves and rejects if a watcher reports an error. + */ +export async function watchAndRegenerate( config: any, workDir: string, logger: Logger, - generate: () => Promise<GenerateResult> + generate: () => Promise<GenerateResult>, + dependencies: WatchDependencies = {} ): Promise<never> { + const watch = dependencies.watch ?? fs.watch; + const debounceMs = dependencies.debounceMs ?? WATCH_DEBOUNCE_MS; let timer: NodeJS.Timeout | undefined; let running = false; let queued = false; const schedule = (changedPath: string): void => { if (timer) clearTimeout(timer); - timer = setTimeout(() => void regenerate(changedPath), 200); + timer = setTimeout(() => void regenerate(changedPath), debounceMs); }; const regenerate = async (changedPath: string): Promise<void> => { @@ -147,7 +172,7 @@ async function watchAndRegenerate( const watchers = config.languages.map((language: any) => { const source = path.resolve(workDir, language.source); - return fs.watch(source, { recursive: true }, (_event, fileName) => { + return watch(source, { recursive: true }, (_event, fileName) => { schedule(fileName?.toString() ?? language.source); }); }); diff --git a/packages/cli/src/commands/generate.watch.test.ts b/packages/cli/src/commands/generate.watch.test.ts new file mode 100644 index 0000000..76f3e14 --- /dev/null +++ b/packages/cli/src/commands/generate.watch.test.ts @@ -0,0 +1,159 @@ +import * as path from "node:path"; +import type { GenerateResult, Logger } from "@docgen/core"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { watchAndRegenerate } from "./generate"; + +// AUDIT-001 / RS-06: watch mode regenerates after a debounce and never runs two generations at once. +// A fake watcher stands in for fs.watch, and fake timers for the debounce. + +type Listener = (event: string, file: string | null) => void; + +function fakeWatcher() { + const listeners: Listener[] = []; + const errorHandlers: Array<(error: Error) => void> = []; + const roots: Array<{ dir: string; recursive: boolean | undefined }> = []; + const watch = vi.fn((dir: string, options: { recursive?: boolean }, listener: Listener) => { + roots.push({ dir, recursive: options.recursive }); + listeners.push(listener); + const watcher = { + on: vi.fn((event: string, handler: (error: Error) => void) => { + if (event === "error") errorHandlers.push(handler); + return watcher; + }), + }; + return watcher; + }); + return { + watch: watch as never, + roots, + emit: (file: string | null = "src/a.ts") => { + for (const listener of listeners) listener("change", file); + }, + fail: (error: Error) => { + for (const handler of errorHandlers) handler(error); + }, + }; +} + +function loggerSpy(): Logger & { messages: string[]; errors: string[] } { + const messages: string[] = []; + const errors: string[] = []; + return { + messages, + errors, + debug() {}, + info: (message: string) => void messages.push(message), + warn() {}, + error: (message: string) => void errors.push(message), + success() {}, + } as never; +} + +const config = { languages: [{ source: "src" }] }; +const result = {} as GenerateResult; + +describe("watchAndRegenerate", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it("watches every language source root recursively", () => { + const fake = fakeWatcher(); + const logger = loggerSpy(); + void watchAndRegenerate( + { languages: [{ source: "src/client" }, { source: "src/server/java" }] }, + "/work", + logger, + async () => result, + { watch: fake.watch }, + ); + + expect(fake.roots).toEqual([ + { dir: path.resolve("/work", "src/client"), recursive: true }, + { dir: path.resolve("/work", "src/server/java"), recursive: true }, + ]); + expect(logger.messages.join("\n")).toContain("Watching 2 source root(s)"); + }); + + it("waits for the debounce, then regenerates once for a burst of changes", async () => { + const fake = fakeWatcher(); + const generate = vi.fn(async () => result); + void watchAndRegenerate(config, "/work", loggerSpy(), generate, { watch: fake.watch, debounceMs: 200 }); + + fake.emit(); + await vi.advanceTimersByTimeAsync(100); + fake.emit(); + await vi.advanceTimersByTimeAsync(100); + fake.emit(); + await vi.advanceTimersByTimeAsync(199); + expect(generate).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(1); + expect(generate).toHaveBeenCalledTimes(1); + }); + + it("uses a 200 ms debounce by default", async () => { + const fake = fakeWatcher(); + const generate = vi.fn(async () => result); + void watchAndRegenerate(config, "/work", loggerSpy(), generate, { watch: fake.watch }); + + fake.emit(); + await vi.advanceTimersByTimeAsync(199); + expect(generate).not.toHaveBeenCalled(); + await vi.advanceTimersByTimeAsync(1); + expect(generate).toHaveBeenCalledTimes(1); + }); + + it("never runs two generations at once: a change during a run is queued and run afterwards", async () => { + const fake = fakeWatcher(); + let finishFirst: () => void = () => undefined; + const generate = vi + .fn<[], Promise<GenerateResult>>() + .mockImplementationOnce(() => new Promise<GenerateResult>((resolve) => (finishFirst = () => resolve(result)))) + .mockResolvedValue(result); + void watchAndRegenerate(config, "/work", loggerSpy(), generate, { watch: fake.watch, debounceMs: 200 }); + + fake.emit(); + await vi.advanceTimersByTimeAsync(200); + expect(generate).toHaveBeenCalledTimes(1); + + fake.emit(); + await vi.advanceTimersByTimeAsync(200); + expect(generate).toHaveBeenCalledTimes(1); // still the first run: the change is queued + + finishFirst(); + await vi.advanceTimersByTimeAsync(200); + expect(generate).toHaveBeenCalledTimes(2); + }); + + it("logs a failed generation and keeps watching", async () => { + const fake = fakeWatcher(); + const logger = loggerSpy(); + const generate = vi + .fn<[], Promise<GenerateResult>>() + .mockRejectedValueOnce(new Error("parse exploded")) + .mockResolvedValue(result); + void watchAndRegenerate(config, "/work", logger, generate, { watch: fake.watch, debounceMs: 200 }); + + fake.emit(); + await vi.advanceTimersByTimeAsync(200); + expect(logger.errors).toEqual(["parse exploded"]); + + fake.emit(); + await vi.advanceTimersByTimeAsync(200); + expect(generate).toHaveBeenCalledTimes(2); + }); + + it("rejects when a watcher reports an error", async () => { + const fake = fakeWatcher(); + const done = watchAndRegenerate(config, "/work", loggerSpy(), async () => result, { watch: fake.watch }); + const outcome = expect(done).rejects.toThrow("watch limit reached"); + + fake.fail(new Error("watch limit reached")); + + await outcome; + }); +}); diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index 8c22feb..54d34b8 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -23,7 +23,7 @@ program program .command("generate") .description("Generate documentation from source code") - .option("-f, --format <formats...>", "Output format(s): markdown, html, pdf, confluence") + .option("-f, --format <formats...>", "Output format(s): markdown, html, pdf") .option("-o, --output <dir>", "Override output directory") .option("--mode <mode>", "Documentation mode: developer or exhaustive") .option("--json", "Output result as JSON (for CI pipelines)") @@ -50,7 +50,7 @@ program program .command("adr") .description("Manage Architecture Decision Records") - .argument("<action>", "Action: new, list, update") + .argument("<action>", "Action: new, list") .argument("[title]", "ADR title (for 'new' action)") .option("-s, --status <status>", "ADR status", "proposed") .action(adrCommand); diff --git a/packages/core/src/orchestrator.outputs.test.ts b/packages/core/src/orchestrator.outputs.test.ts new file mode 100644 index 0000000..2d8e60a --- /dev/null +++ b/packages/core/src/orchestrator.outputs.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it } from "vitest"; +import { DocGenConfigSchema } from "./config/schema"; +import { createSilentLogger } from "./plugin/types"; +import { Orchestrator } from "./orchestrator"; + +// AUDIT-001 / RS-15: the config schema and the CLI accept a Confluence output, but RepoScribe has no +// Confluence renderer. Enabling it must fail with a clear message, not a hint to install a package +// that does not exist. + +function configWith(output: Record<string, unknown>) { + return DocGenConfigSchema.parse({ + project: { name: "Demo" }, + languages: [{ name: "typescript", source: "src", parser: "@docgen/parser-typescript" }], + output, + }); +} + +describe("outputs that have no renderer", () => { + it("rejects an enabled Confluence output with a clear message", async () => { + const orchestrator = new Orchestrator({ + config: configWith({ confluence: { enabled: true } }), + workDir: process.cwd(), + logger: createSilentLogger(), + }); + + await expect(orchestrator.generate()).rejects.toThrow(/Confluence output is not available/); + await expect(orchestrator.generate()).rejects.toThrow(/output\.confluence\.enabled/); + }); + + it("does not suggest installing a package that does not exist", async () => { + const orchestrator = new Orchestrator({ + config: configWith({ confluence: { enabled: true } }), + workDir: process.cwd(), + logger: createSilentLogger(), + }); + + const error = await orchestrator.generate().then( + () => undefined, + (caught: unknown) => caught as Error, + ); + expect(error).toBeInstanceOf(Error); + expect(error?.message).not.toMatch(/npm install/); + expect(error?.message).not.toMatch(/renderer-confluence/); + }); + + it("applies to snapshot and validate too, since they load the same plugins", async () => { + const orchestrator = new Orchestrator({ + config: configWith({ confluence: { enabled: true } }), + workDir: process.cwd(), + logger: createSilentLogger(), + }); + + await expect(orchestrator.snapshot()).rejects.toThrow(/Confluence output is not available/); + await expect(orchestrator.validate()).rejects.toThrow(/Confluence output is not available/); + }); +}); diff --git a/packages/core/src/orchestrator.ts b/packages/core/src/orchestrator.ts index 9180c16..d1dc4ad 100644 --- a/packages/core/src/orchestrator.ts +++ b/packages/core/src/orchestrator.ts @@ -18,6 +18,14 @@ import { LinkResolver } from "./transforms/link-resolver"; // Orchestrator - Coordinates the full parse → transform → render pipeline // ───────────────────────────────────────────────────────────────── +/** + * Output formats the configuration schema accepts but no package renders. Enabling one is an error: + * the plugin loader would otherwise tell the user to install a package that does not exist. + */ +const UNAVAILABLE_OUTPUTS: Record<string, string> = { + confluence: "Confluence output is not available: RepoScribe has no Confluence renderer.", +}; + export interface OrchestratorOptions { config: DocGenConfig; workDir: string; @@ -157,6 +165,16 @@ export class Orchestrator { private async loadAllPlugins(): Promise<PluginRegistry> { if (this.registry) return this.registry; + // Fail early and clearly for outputs the config accepts but nothing implements. + for (const [format, message] of Object.entries(UNAVAILABLE_OUTPUTS)) { + const outputConfig = this.config.output[format as keyof typeof this.config.output]; + if (outputConfig && "enabled" in outputConfig && outputConfig.enabled) { + throw new Error( + `${message} Set output.${format}.enabled to false, or generate markdown, html or pdf.` + ); + } + } + // Collect all plugin names from config const pluginNames = new Set<string>(); @@ -175,7 +193,6 @@ export class Orchestrator { markdown: "@docgen/renderer-markdown", html: "@docgen/renderer-html", pdf: "@docgen/renderer-pdf", - confluence: "@docgen/renderer-confluence", }; for (const [format, pkg] of Object.entries(outputMap)) { diff --git a/packages/parser-java/src/__fixtures__/DocumentedService.java b/packages/parser-java/src/__fixtures__/DocumentedService.java new file mode 100644 index 0000000..4886416 --- /dev/null +++ b/packages/parser-java/src/__fixtures__/DocumentedService.java @@ -0,0 +1,34 @@ +package com.example.service; + +/** Combines values for callers. */ +public class DocumentedService { + /** + * Creates the service. + * @param repository where the values are stored + */ + public DocumentedService(String repository) { + } + + /** + * Adds two numbers. + * @param a the first addend + * @param b the second addend - added to the first + * @param ghost documents a parameter that does not exist + * @return the sum + */ + public int add(int a, int b) { + return a + b; + } + + /** + * Joins values. + * @param parts the parts to join + */ + public String join(String... parts) { + return String.join(",", parts); + } + + public int undocumented(int x) { + return x; + } +} diff --git a/packages/parser-java/src/index.test.ts b/packages/parser-java/src/index.test.ts index e19e627..e28a11a 100644 --- a/packages/parser-java/src/index.test.ts +++ b/packages/parser-java/src/index.test.ts @@ -1,17 +1,26 @@ import * as path from "node:path"; +import { DocGenConfigSchema, createSilentLogger, type PluginConfig } from "@docgen/core"; import { describe, expect, it } from "vitest"; import { JavaParser } from "./index"; +/** A real, schema-validated config and a silent logger, so the tests need no type casts. */ +function pluginConfig(workDir: string): PluginConfig { + return { + projectConfig: DocGenConfigSchema.parse({ + project: { name: "Demo" }, + languages: [{ name: "java", source: "src", parser: "@docgen/parser-java" }], + }), + workDir, + options: {}, + logger: createSilentLogger(), + }; +} + describe("JavaParser", () => { it("parses plain Java types and Spring REST endpoint metadata", async () => { const fixtureRoot = path.join(process.cwd(), "src", "__fixtures__"); const parser = new JavaParser(); - await parser.initialize({ - projectConfig: {} as any, - workDir: fixtureRoot, - options: {}, - logger: console as any, - }); + await parser.initialize(pluginConfig(fixtureRoot)); const docir = await parser.parse( [ @@ -94,4 +103,53 @@ describe("JavaParser", () => { requestBody: expect.objectContaining({ raw: "CreateUserRequest" }), })); }); + + it("attaches Javadoc @param text to each parameter (AUDIT-001 / RS-05)", async () => { + const fixtureRoot = path.join(process.cwd(), "src", "__fixtures__"); + const parser = new JavaParser(); + await parser.initialize(pluginConfig(fixtureRoot)); + + const docir = await parser.parse([path.join(fixtureRoot, "DocumentedService.java")], {}); + const service = docir.modules.find((module) => module.name === "DocumentedService"); + const describe = (member: string) => + Object.fromEntries( + (service?.members.find((candidate) => candidate.name === member)?.parameters ?? []).map((parameter) => [ + parameter.name, + parameter.description, + ]) + ); + + expect(describe("add")).toEqual({ a: "the first addend", b: "the second addend - added to the first" }); + expect(describe("DocumentedService")).toEqual({ repository: "where the values are stored" }); + expect(describe("join")).toEqual({ parts: "the parts to join" }); + expect(describe("undocumented")).toEqual({ x: "" }); + }); + + it("reports a varargs parameter under its own name and as an array of its element type", async () => { + const fixtureRoot = path.join(process.cwd(), "src", "__fixtures__"); + const parser = new JavaParser(); + await parser.initialize(pluginConfig(fixtureRoot)); + + const docir = await parser.parse([path.join(fixtureRoot, "DocumentedService.java")], {}); + const join = docir.modules[0]?.members.find((member) => member.name === "join"); + + expect(join?.parameters).toHaveLength(1); + expect(join?.parameters[0]).toMatchObject({ + name: "parts", + isRest: true, + type: { raw: "String[]", name: "String", isArray: true }, + }); + }); + + it("keeps the @param tags on the member as well", async () => { + const fixtureRoot = path.join(process.cwd(), "src", "__fixtures__"); + const parser = new JavaParser(); + await parser.initialize(pluginConfig(fixtureRoot)); + + const docir = await parser.parse([path.join(fixtureRoot, "DocumentedService.java")], {}); + const add = docir.modules[0]?.members.find((member) => member.name === "add"); + + expect(add?.tags.filter((tag) => tag.tag === "param").map((tag) => tag.name)).toEqual(["a", "b", "ghost"]); + expect(add?.parameters.map((parameter) => parameter.name)).toEqual(["a", "b"]); + }); }); diff --git a/packages/parser-java/src/index.ts b/packages/parser-java/src/index.ts index 856d8f6..d3c1529 100644 --- a/packages/parser-java/src/index.ts +++ b/packages/parser-java/src/index.ts @@ -244,22 +244,46 @@ export class JavaParser implements ParserPlugin { private parseParameters(parametersNode: SyntaxNode | null): ParamNode[] { if (!parametersNode) return []; + // @param tags are in the Javadoc of the method or constructor that owns the parameter list. + const owner = parametersNode.parent; + const docs = owner ? this.javadoc(owner) : null; return parametersNode.namedChildren .filter((node) => node.type === "formal_parameter" || node.type === "spread_parameter") .map((node) => { - const name = node.childForFieldName("name")?.text ?? "arg"; - const docs = this.javadoc(node.parent ?? node); + const { name, typeText } = this.parameterNameAndType(node); return { name, - type: this.typeRef(node.childForFieldName("type")?.text ?? "Object"), + type: this.typeRef(typeText), description: - docs.tags.find((tag) => tag.tag === "param" && tag.name === name)?.description ?? "", + docs?.tags.find((tag) => tag.tag === "param" && tag.name === name)?.description ?? "", isOptional: false, isRest: node.type === "spread_parameter", }; }); } + /** + * A `formal_parameter` exposes `name` and `type` fields. A varargs `spread_parameter` does not: its + * name is inside a `variable_declarator` child and its type is the first other named child. A varargs + * parameter is an array of that type. + */ + private parameterNameAndType(node: SyntaxNode): { name: string; typeText: string } { + if (node.type !== "spread_parameter") { + return { + name: node.childForFieldName("name")?.text ?? "arg", + typeText: node.childForFieldName("type")?.text ?? "Object", + }; + } + const declarator = node.namedChildren.find((child) => child.type === "variable_declarator"); + const elementType = node.namedChildren.find( + (child) => child.type !== "modifiers" && child.type !== "variable_declarator" + ); + return { + name: declarator?.childForFieldName("name")?.text ?? "arg", + typeText: elementType ? `${elementType.text}[]` : "Object", + }; + } + private extractEndpoint( annotations: DecoratorNode[], parameters: ParamNode[], diff --git a/packages/parser-typescript/src/cache.test.ts b/packages/parser-typescript/src/cache.test.ts new file mode 100644 index 0000000..cba1ac7 --- /dev/null +++ b/packages/parser-typescript/src/cache.test.ts @@ -0,0 +1,102 @@ +import * as fs from "fs"; +import * as path from "path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@docgen/core", async () => await import("../../core/src")); + +// AUDIT-001 / RS-07: the parser caches parsed modules per file, keyed on modification time and size, +// so watch mode does not re-parse files that have not changed. `parseSourceFile` is the expensive step; +// spying on it shows exactly when a file is parsed and when it is served from the cache. + +const langConfig = { + name: "typescript", + source: "src", + include: ["**/*.ts"], + exclude: [], + parser: "@docgen/parser-typescript", + options: {}, +}; + +describe("TypeScriptParser module cache", () => { + let dir: string; + let a: string; + let b: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(process.cwd(), ".tmp-parser-cache-")); + fs.mkdirSync(path.join(dir, "src"), { recursive: true }); + a = path.join(dir, "src", "a.ts"); + b = path.join(dir, "src", "b.ts"); + fs.writeFileSync(a, "export function alpha() { return 1; }\n"); + fs.writeFileSync(b, "export function bravo() { return 2; }\n"); + }); + + afterEach(() => { + vi.restoreAllMocks(); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + async function newParser() { + const { TypeScriptParser } = await import("./index"); + const spy = vi.spyOn(TypeScriptParser.prototype as never, "parseSourceFile"); + return { parser: new TypeScriptParser(), spy }; + } + + const names = (docir: { modules: Array<{ name: string }> }) => docir.modules.map((module) => module.name).sort(); + + it("parses each file once, then serves unchanged files from the cache", async () => { + const { parser, spy } = await newParser(); + + const first = await parser.parse([a, b], langConfig); + expect(spy).toHaveBeenCalledTimes(2); + + const second = await parser.parse([a, b], langConfig); + expect(spy).toHaveBeenCalledTimes(2); // no further parsing + expect(names(second)).toEqual(["alpha", "bravo"]); + expect(second.modules).toEqual(first.modules); + }); + + it("re-parses only the file that changed and reports its new content", async () => { + const { parser, spy } = await newParser(); + await parser.parse([a, b], langConfig); + + fs.writeFileSync(b, "export function bravoRenamed() { return 22; }\n"); + const docir = await parser.parse([a, b], langConfig); + + expect(spy).toHaveBeenCalledTimes(3); // a from cache, b parsed again + expect(names(docir)).toEqual(["alpha", "bravoRenamed"]); + }); + + it("treats a changed modification time as a change even when the size is the same", async () => { + const { parser, spy } = await newParser(); + await parser.parse([a], langConfig); + + fs.writeFileSync(a, "export function omega() { return 1; }\n"); // same length as before + const later = new Date(Date.now() + 60_000); + fs.utimesSync(a, later, later); + const docir = await parser.parse([a], langConfig); + + expect(spy).toHaveBeenCalledTimes(2); + expect(names(docir)).toEqual(["omega"]); + }); + + it("forgets files that are no longer in the file list", async () => { + const { parser, spy } = await newParser(); + await parser.parse([a, b], langConfig); + await parser.parse([a], langConfig); // b leaves the list: its entry is evicted + expect(spy).toHaveBeenCalledTimes(2); + + await parser.parse([a, b], langConfig); // b returns: parsed again, not served from a stale entry + expect(spy).toHaveBeenCalledTimes(3); + }); + + it("hands out copies, so changing a result cannot corrupt the cache", async () => { + const { parser } = await newParser(); + const first = await parser.parse([a], langConfig); + first.modules[0].name = "tampered"; + + const second = await parser.parse([a], langConfig); + + expect(names(second)).toEqual(["alpha"]); + }); +}); diff --git a/packages/parser-typescript/src/index.ts b/packages/parser-typescript/src/index.ts index e4270ec..fb30bb6 100644 --- a/packages/parser-typescript/src/index.ts +++ b/packages/parser-typescript/src/index.ts @@ -281,7 +281,7 @@ export class TypeScriptParser implements ParserPlugin { isAbstract: false, signature: method.getText(), description: this.extractDescription(method), - parameters: this.extractParameters(method.getParameters()), + parameters: this.extractParameters(method.getParameters(), method), returnType: this.buildTypeRef(method.getReturnType()), throws: [], deprecated: null, @@ -440,7 +440,7 @@ export class TypeScriptParser implements ParserPlugin { isAbstract: false, signature: this.buildFunctionSignature(func), description: this.extractDescription(func), - parameters: this.extractParameters(func.getParameters()), + parameters: this.extractParameters(func.getParameters(), func), returnType: this.buildTypeRef(func.getReturnType()), throws: this.extractThrows(func), deprecated: null, @@ -498,7 +498,7 @@ export class TypeScriptParser implements ParserPlugin { isAbstract: false, signature: this.buildVariableFunctionSignature(declaration, initializer), description: this.extractDescription(docNode), - parameters: this.extractParameters(initializer.getParameters()), + parameters: this.extractParameters(initializer.getParameters(), docNode), returnType: this.buildTypeRef(initializer.getReturnType()), throws: this.extractThrows(docNode), deprecated: this.extractDeprecation(docNode), @@ -715,7 +715,7 @@ export class TypeScriptParser implements ParserPlugin { isAbstract: method.isAbstract(), signature: this.buildMethodSignature(method), description: this.extractDescription(method), - parameters: this.extractParameters(method.getParameters()), + parameters: this.extractParameters(method.getParameters(), method), returnType: this.buildTypeRef(method.getReturnType()), throws: this.extractThrows(method), deprecated: this.extractDeprecation(method), @@ -761,7 +761,7 @@ export class TypeScriptParser implements ParserPlugin { .map((p) => p.getText()) .join(", ")})`, description: this.extractDescription(ctor), - parameters: this.extractParameters(ctor.getParameters()), + parameters: this.extractParameters(ctor.getParameters(), ctor), returnType: null, throws: [], deprecated: null, @@ -787,7 +787,8 @@ export class TypeScriptParser implements ParserPlugin { parameters: kind === "setter" ? this.extractParameters( - (accessor as SetAccessorDeclaration).getParameters() + (accessor as SetAccessorDeclaration).getParameters(), + accessor ) : [], returnType: @@ -904,32 +905,38 @@ export class TypeScriptParser implements ParserPlugin { return text.replace(/import\([^)]+\)\./g, ""); } - private extractParameters(params: ParameterDeclaration[]): ParamNode[] { + /** + * Build parameter nodes for a callable. + * @param params - The declared parameters, in order. + * @param docNode - The node that carries the JSDoc block: the function itself, or the variable + * statement for an arrow function assigned to a const. + */ + private extractParameters(params: ParameterDeclaration[], docNode: Node): ParamNode[] { return params.map((p) => ({ name: p.getName(), type: this.buildTypeRef(p.getType()), - description: this.getParamDescription(p), + description: this.getParamDescription(p, docNode), isOptional: p.isOptional(), isRest: p.isRestParameter(), defaultValue: p.getInitializer()?.getText(), })); } - private getParamDescription(param: ParameterDeclaration): string { - // Walk up to the function/method and find matching @param tag - const parent = param.getParent(); - if (!parent) return ""; - - const tags = this.extractTags(parent); - const paramTag = tags.find( - (t) => t.tag === "param" && t.description.startsWith(param.getName()) - ); - - if (paramTag) { - // Strip the param name from the value - return paramTag.description.replace(new RegExp(`^${param.getName()}\\s*-?\\s*`), ""); + /** + * Find the `@param` text for one parameter. A tag's name is matched exactly (so `id` never picks up + * `idx`'s text), and the optional `{type}`, the name and a leading hyphen are not part of the text. + */ + private getParamDescription(param: ParameterDeclaration, docNode: Node): string { + const name = param.getName(); + for (const doc of this.getJsDocs(docNode)) { + for (const tag of doc.getTags()) { + if (!Node.isJSDocParameterTag(tag) || tag.getName() !== name) continue; + return (tag.getCommentText() ?? "") + .replace(/^\s*-\s*/, "") + .replace(/\s*\n\s*/g, " ") + .trim(); + } } - return ""; } diff --git a/packages/parser-typescript/src/param-docs.test.ts b/packages/parser-typescript/src/param-docs.test.ts new file mode 100644 index 0000000..e54aa16 --- /dev/null +++ b/packages/parser-typescript/src/param-docs.test.ts @@ -0,0 +1,115 @@ +import * as fs from "fs"; +import * as path from "path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@docgen/core", async () => await import("../../core/src")); + +// Each parse builds a ts-morph project; the first one in a cold process is slow. +vi.setConfig({ testTimeout: 30_000 }); + +// AUDIT-001 follow-up: `@param name - text` tags did not reach ParamNode.description, so RepoScribe's own +// validation listed every documented parameter as undocumented. These tests pin the intended behaviour. + +const langConfig = { + name: "typescript", + source: "src", + include: ["**/*.ts"], + exclude: [], + parser: "@docgen/parser-typescript", + options: {}, +}; + +describe("TypeScriptParser @param descriptions", () => { + let dir: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(process.cwd(), ".tmp-param-docs-")); + fs.mkdirSync(path.join(dir, "src"), { recursive: true }); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + async function parameters(source: string, memberName: string): Promise<Record<string, string>> { + const file = path.join(dir, "src", "sample.ts"); + fs.writeFileSync(file, source); + const { TypeScriptParser } = await import("./index"); + const docir = await new TypeScriptParser().parse([file], langConfig); + const members = docir.modules.flatMap((module) => module.members); + const member = members.find((candidate) => candidate.name === memberName); + if (!member) throw new Error(`member ${memberName} not found in ${members.map((m) => m.name).join(", ")}`); + return Object.fromEntries(member.parameters.map((parameter) => [parameter.name, parameter.description])); + } + + it("reads a description written with a hyphen after the name", async () => { + const result = await parameters( + ["/**", " * Add.", " * @param a - The first number.", " * @param b - The second number.", " */", + "export function add(a: number, b: number): number { return a + b; }"].join("\n"), + "add" + ); + expect(result).toEqual({ a: "The first number.", b: "The second number." }); + }); + + it("reads a description written without a hyphen", async () => { + const result = await parameters( + ["/**", " * Add.", " * @param a The first number.", " */", "export function add(a: number): number { return a; }"].join("\n"), + "add" + ); + expect(result).toEqual({ a: "The first number." }); + }); + + it("ignores a {type} annotation in the tag", async () => { + const result = await parameters( + ["/**", " * Add.", " * @param {number} a - The first number.", " */", "export function add(a: number): number { return a; }"].join("\n"), + "add" + ); + expect(result).toEqual({ a: "The first number." }); + }); + + it("gives each parameter its own text when names share a prefix", async () => { + const result = await parameters( + ["/**", " * Find.", " * @param idx - The index.", " * @param id - The identifier.", " */", + "export function find(id: string, idx: number): string { return id + idx; }"].join("\n"), + "find" + ); + expect(result).toEqual({ id: "The identifier.", idx: "The index." }); + }); + + it("leaves an undocumented parameter empty instead of borrowing another tag", async () => { + const result = await parameters( + ["/**", " * Add.", " * @param a - The first number.", " */", + "export function add(a: number, b: number): number { return a + b; }"].join("\n"), + "add" + ); + expect(result).toEqual({ a: "The first number.", b: "" }); + }); + + it("keeps a multi-line description together", async () => { + const result = await parameters( + ["/**", " * Add.", " * @param a - The first number,", " * which may be negative.", " */", + "export function add(a: number): number { return a; }"].join("\n"), + "add" + ); + expect(result.a).toContain("The first number,"); + expect(result.a).toContain("which may be negative."); + }); + + it("reads descriptions on arrow functions assigned to a const", async () => { + const result = await parameters( + ["/**", " * Double.", " * @param n - The number to double.", " */", + "export const double = (n: number): number => n * 2;"].join("\n"), + "double" + ); + expect(result).toEqual({ n: "The number to double." }); + }); + + it("reads descriptions on class methods", async () => { + const result = await parameters( + ["export class Calculator {", " /**", " * Add.", " * @param a - Left side.", " * @param b - Right side.", " */", + " add(a: number, b: number): number { return a + b; }", "}"].join("\n"), + "add" + ); + expect(result).toEqual({ a: "Left side.", b: "Right side." }); + }); +});