diff --git a/TESTING.md b/TESTING.md new file mode 100644 index 0000000..c84560a --- /dev/null +++ b/TESTING.md @@ -0,0 +1,36 @@ +# Real-world accuracy test + +This documents a real-world test of seal against a fork of [tj/commander.js](https://github.com/tj/commander.js) (a real, actively-maintained, well-documented JavaScript library — chosen over the originally-suggested FastAPI/Pydantic for its much smaller size, to keep the test's embedding-API cost and runtime tractable on a free-tier key). + +## Methodology + +1. Forked `tj/commander.js` to `dakshcodez/commander.js` and cloned it locally. +2. Ran `seal index` for real (live Gemini API, `gemini-3.7-flash`/`gemini-embedding-2` for indexing, `gemini-3.5-flash-lite` for the staleness/repair calls during this test session specifically to work around `3.7-flash`'s demand-driven `503`s — the shipped default remains `3.7-flash`). Indexed the full repo: **232 code chunks, 365 doc sections** (before the changelog-exclusion fix described below), producing 194 links (178 heuristic, 16 embedding). +3. Made three deliberate, targeted code changes and staged them, then ran `seal check` for real against the live pipeline. +4. Inspected the actual generated corrections and verdicts against expectations. + +## Deliberate test cases + +| # | Change | Expected | Actual result | +|---|---|---|---| +| 1 | Renamed `Command.prototype.name()` → `.setName()` in `lib/command.js` and `typings/index.d.ts` (breaking signature change, docs untouched) | Flag `Readme.md`'s `.name` section as stale | **Correctly flagged and auto-fixed** — `Readme.md#Commander.js > Automated help > .name` | +| 2 | Added an explanatory comment inside `.help()`'s body, no logic change | Not flagged | **Correctly not flagged** — filtered out before ever reaching the LLM (AST-based comment stripping) | +| 3 | Rewrote the internal, undocumented `_getCommandAndAncestors()` helper's loop style (`for` → `while`), same behavior, zero doc links | Not flagged | **Correctly produced zero suspects** — no doc section is linked to it at all | +| 4 (unplanned) | Same rename from #1 also touched a second, more ambiguous section (`Bits and pieces > Legacy options as properties`) referencing `.name()` in a way that was only partly affected | Ambiguous | The generation pass proposed a correction; the **validation pass caught that it was actually wrong** (the typings still exposed a valid `.name()` getter alongside the new `.setName()` setter) and downgraded it to "needs review" instead of auto-applying — the two-pass generate→validate design working exactly as intended | + +## Bugs found and fixed as a direct result of this test + +Live testing against a real repository surfaced three real bugs that no amount of mocked testing had caught: + +1. **Indexing crashed outright** on a real repo: two locally-scoped test helper functions named `makeProgram` in different `describe()` blocks of the same test file produced identical chunk IDs, and Vectra's `insertItem` throws on a duplicate ID. Fixed two ways: test files are now excluded from code parsing entirely (they were never a useful doc-linking target anyway), and the embedding index now upserts rather than inserts, so any remaining ID collision degrades gracefully instead of crashing. +2. **No retry/backoff on rate limits.** The free tier's per-minute quotas (100 embed requests/min, 15 generate requests/min for `gemini-3.5-flash-lite`) were hit repeatedly during real indexing and staleness-checking, and every `429`/`503` was previously fatal. Added retry-with-exponential-backoff (up to 4 attempts) to `GeminiClient`. +3. **Changelog files were incorrectly treated as living documentation.** `CHANGELOG.md` entries describing historical releases (e.g. a 2020 entry for `.parseOption()`) got "corrected" to reflect current code state — rewriting history, not fixing stale docs. Fixed by excluding changelog-style files (`CHANGELOG.md`, `CHANGES.md`, `HISTORY.md`, `RELEASES.md`, case-insensitive) from doc parsing entirely, the same way test files are excluded from code parsing. + +Also observed, not fixed: the correction pass occasionally touched unrelated formatting in the same file (removed a blank line after a few `### Added` headings in `CHANGELOG.md` before that file was excluded) despite being instructed to preserve untouched content — a minor prompt-adherence gap worth revisiting. + +## Known limitations of this test + +- The changelog-exclusion fix (bug #3 above) is verified at the unit level (the file-matching pattern was tested directly against 7 real-world filename cases, all correct) but **not re-validated end-to-end against the live API** — the free tier's *daily* embedding quota (1000 requests/day) was exhausted while re-indexing to confirm it, and daily quotas don't reset within a session. A full end-to-end re-run is a natural follow-up once quota is available. +- This test exercised the `@seal/cli` path (`seal index` + `seal check`) only, not the GitHub Action — the Action's `dist/index.js` isn't bundled for standalone execution yet (see the publish-phase task). +- One real repository, one language (JavaScript), a handful of deliberate cases — not a statistically rigorous accuracy benchmark, but real signal from real code and real docs rather than synthetic fixtures. +- The new `isTestFile`/`isChangelogFile` exclusions are unconditional — there's no way to opt a specific file back in short of renaming it. `.sealignore` exists as a user-facing override mechanism, but it's currently only wired into `check.ts`'s changed-file filtering, not into the doc/code indexing walk itself (a pre-existing gap, not introduced by this fix). A team that genuinely wants a file named `HISTORY.md` treated as living documentation has no escape hatch today. diff --git a/packages/core/src/changes/filter.ts b/packages/core/src/changes/filter.ts index 7ecca77..a31e878 100644 --- a/packages/core/src/changes/filter.ts +++ b/packages/core/src/changes/filter.ts @@ -1,13 +1,10 @@ import type { Node } from 'web-tree-sitter'; import type { GrammarId } from '../parsing/index.js'; import { createParser, resolveGrammar } from '../parsing/index.js'; +import { isTestFile } from '../shared/is-test-file.js'; import type { ChunkDiff } from './types.js'; -const TEST_FILE_PATTERN = /(^|\/)(__tests__|tests?)\/|\.(test|spec)\.[^/]+$/; - -export function isTestFile(filePath: string): boolean { - return TEST_FILE_PATTERN.test(filePath); -} +export { isTestFile }; function collectCommentRanges(node: Node, ranges: [number, number][]): void { if (node.type === 'comment') { diff --git a/packages/core/src/docs/file-walker.ts b/packages/core/src/docs/file-walker.ts index 23e264a..1cd194f 100644 --- a/packages/core/src/docs/file-walker.ts +++ b/packages/core/src/docs/file-walker.ts @@ -1,11 +1,18 @@ import { extname } from 'node:path'; import { walkFiles } from '../shared/walk-files.js'; +const CHANGELOG_FILE_PATTERN = /(^|\/)(changelog|changes|history|releases)\.md$/i; + export interface DocFile { absolutePath: string; relativePath: string; } +export function isChangelogFile(filePath: string): boolean { + return CHANGELOG_FILE_PATTERN.test(filePath); +} + export async function walkDocFiles(rootDir: string): Promise { - return walkFiles(rootDir, (name) => extname(name) === '.md'); + const files = await walkFiles(rootDir, (name) => extname(name) === '.md'); + return files.filter((file) => !isChangelogFile(file.relativePath)); } diff --git a/packages/core/src/docs/index.ts b/packages/core/src/docs/index.ts index ffe93e4..4e408c4 100644 --- a/packages/core/src/docs/index.ts +++ b/packages/core/src/docs/index.ts @@ -3,5 +3,5 @@ export { applySectionCorrection } from './apply-correction.js'; export { extractCodeReferences } from './code-references.js'; export { parseMarkdownSections } from './markdown-parser.js'; export { parseDocs } from './parser.js'; -export { walkDocFiles } from './file-walker.js'; +export { isChangelogFile, walkDocFiles } from './file-walker.js'; export type { DocFile } from './file-walker.js'; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 4322284..7bf81aa 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -10,6 +10,7 @@ export type { DocFile, DocSection } from './docs/index.js'; export { applySectionCorrection, extractCodeReferences, + isChangelogFile, parseDocs, parseMarkdownSections, walkDocFiles, diff --git a/packages/core/src/linkgraph/embedding.ts b/packages/core/src/linkgraph/embedding.ts index 3cfc756..89f6c73 100644 --- a/packages/core/src/linkgraph/embedding.ts +++ b/packages/core/src/linkgraph/embedding.ts @@ -36,9 +36,19 @@ export async function buildEmbeddingLinks( } await index.createIndex({ version: 1 }); + const seenChunkIds = new Set(); for (const chunk of chunks) { const vector = await llm.embed(chunkEmbeddingText(chunk)); - await index.insertItem({ id: chunk.id, vector }); + // upsert, not insert: two chunks can legitimately share an id (e.g. + // same-named locally-scoped helpers in different function bodies) - + // that's an inherent limit of a name-based id scheme, not a fatal error. + // Still surfaced as a warning, since it means one of the two silently + // becomes unlinkable rather than crashing loudly like insertItem did. + if (seenChunkIds.has(chunk.id)) { + console.warn(`seal: duplicate chunk id "${chunk.id}" - only the last one is linkable in the embedding index.`); + } + seenChunkIds.add(chunk.id); + await index.upsertItem({ id: chunk.id, vector }); } const links: Link[] = []; diff --git a/packages/core/src/llm/gemini-client.ts b/packages/core/src/llm/gemini-client.ts index de6b51e..ef58e98 100644 --- a/packages/core/src/llm/gemini-client.ts +++ b/packages/core/src/llm/gemini-client.ts @@ -1,4 +1,4 @@ -import { GoogleGenAI } from '@google/genai'; +import { ApiError, GoogleGenAI } from '@google/genai'; import type { CompleteOptions, LLMClient } from './types.js'; export interface GeminiClientOptions { @@ -10,6 +10,38 @@ export interface GeminiClientOptions { const DEFAULT_GENERATION_MODEL = 'gemini-3.7-flash'; const DEFAULT_EMBEDDING_MODEL = 'gemini-embedding-2'; +const MAX_RETRIES = 4; +const INITIAL_BACKOFF_MS = 1000; +const RETRYABLE_ERROR_CODES = new Set(['ECONNRESET', 'ETIMEDOUT', 'ECONNREFUSED', 'ENOTFOUND', 'EAI_AGAIN']); + +function isRetryable(error: unknown): boolean { + if (error instanceof ApiError) { + return error.status === 429 || error.status === 503; + } + // The SDK only wraps HTTP-level failures into ApiError - a raw fetch() + // failure (DNS, connection reset, timeout) surfaces as a plain network + // error instead, and is just as worth retrying in a CI environment. + if (error instanceof Error) { + if (error.name === 'AbortError') return true; + if (error instanceof TypeError && error.cause) return true; + const code = (error as NodeJS.ErrnoException).code; + if (code && RETRYABLE_ERROR_CODES.has(code)) return true; + } + return false; +} + +async function withRetry(fn: () => Promise): Promise { + for (let attempt = 0; ; attempt += 1) { + try { + return await fn(); + } catch (error) { + if (!isRetryable(error) || attempt >= MAX_RETRIES) throw error; + const delayMs = INITIAL_BACKOFF_MS * 2 ** attempt; + await new Promise((resolve) => setTimeout(resolve, delayMs)); + } + } +} + export class GeminiClient implements LLMClient { private readonly client: GoogleGenAI; private readonly generationModel: string; @@ -29,15 +61,17 @@ export class GeminiClient implements LLMClient { } async complete(prompt: string, options: CompleteOptions = {}): Promise { - const response = await this.client.models.generateContent({ - model: this.generationModel, - contents: prompt, - config: { - systemInstruction: options.systemInstruction, - temperature: options.temperature, - maxOutputTokens: options.maxOutputTokens, - }, - }); + const response = await withRetry(() => + this.client.models.generateContent({ + model: this.generationModel, + contents: prompt, + config: { + systemInstruction: options.systemInstruction, + temperature: options.temperature, + maxOutputTokens: options.maxOutputTokens, + }, + }), + ); const text = response.text; if (text === undefined) { @@ -47,10 +81,12 @@ export class GeminiClient implements LLMClient { } async embed(text: string): Promise { - const response = await this.client.models.embedContent({ - model: this.embeddingModel, - contents: text, - }); + const response = await withRetry(() => + this.client.models.embedContent({ + model: this.embeddingModel, + contents: text, + }), + ); const values = response.embeddings?.[0]?.values; if (!values) { diff --git a/packages/core/src/parsing/file-walker.ts b/packages/core/src/parsing/file-walker.ts index de995d9..5ecbdbe 100644 --- a/packages/core/src/parsing/file-walker.ts +++ b/packages/core/src/parsing/file-walker.ts @@ -1,4 +1,5 @@ import { extname } from 'node:path'; +import { isTestFile } from '../shared/is-test-file.js'; import { walkFiles } from '../shared/walk-files.js'; import type { GrammarId } from './types.js'; @@ -24,8 +25,10 @@ export function resolveGrammar(filePath: string): GrammarId | undefined { export async function walkSourceFiles(rootDir: string): Promise { const files = await walkFiles(rootDir, (name) => extname(name) in EXTENSION_TO_GRAMMAR); - return files.map((file) => ({ - ...file, - grammar: EXTENSION_TO_GRAMMAR[extname(file.relativePath)], - })); + return files + .filter((file) => !isTestFile(file.relativePath)) + .map((file) => ({ + ...file, + grammar: EXTENSION_TO_GRAMMAR[extname(file.relativePath)], + })); } diff --git a/packages/core/src/shared/is-test-file.ts b/packages/core/src/shared/is-test-file.ts new file mode 100644 index 0000000..e0c05ca --- /dev/null +++ b/packages/core/src/shared/is-test-file.ts @@ -0,0 +1,5 @@ +const TEST_FILE_PATTERN = /(^|\/)(__tests__|tests?)\/|\.(test|spec)\.[^/]+$/; + +export function isTestFile(filePath: string): boolean { + return TEST_FILE_PATTERN.test(filePath); +}