From e63aada41620aaf8f54c21290efbd60c65954daa Mon Sep 17 00:00:00 2001 From: dakshcodez Date: Fri, 14 Aug 2026 23:41:41 +0530 Subject: [PATCH 1/2] Test seal end-to-end against a real repository, fix 3 real bugs found Implements Phase 5 (real-world accuracy test). Forked tj/commander.js (picked over the originally-suggested FastAPI/Pydantic for its much smaller size, keeping embedding-API cost/time tractable on a free-tier key), ran seal index and seal check for real against the live Gemini API, and made three deliberate code changes to measure accuracy. Full methodology and results in TESTING.md. Results: correctly flagged and auto-fixed a real breaking rename (.name() -> .setName()); correctly ignored a comment-only change and an undocumented internal refactor (both never reached the LLM); the validation pass caught and downgraded a flawed initial correction to "needs review" instead of wrongly auto-applying it, on a genuinely ambiguous real case - the two-pass generate/validate design working exactly as intended, not just in mocked tests. Live testing surfaced three real bugs no mocked test had caught: - shared/is-test-file.ts (moved from changes/filter.ts) + parsing/ file-walker.ts: indexing crashed outright on commander.js's real test suite - two locally-scoped test helpers both named `makeProgram` in different describe() blocks produced identical chunk ids, and Vectra's insertItem throws on a duplicate id. Test files are now excluded from code parsing entirely (they were never a useful doc-linking target anyway). linkgraph/embedding.ts also switches insertItem to upsertItem as defense in depth, so any remaining id collision degrades gracefully instead of crashing. - llm/gemini-client.ts: real indexing and staleness-checking hit the free tier's per-minute quotas repeatedly (100 embed requests/min, 15 generate requests/min for gemini-3.5-flash-lite), and every 429/503 was previously fatal, aborting the whole run. Added retry-with-exponential-backoff (up to 4 attempts). - docs/file-walker.ts: CHANGELOG.md entries describing historical releases got "corrected" to match current code state - e.g. a 2020 entry for `.parseOption()` rewritten to describe a later rename that has nothing to do with the actual deliberate test change, silently rewriting history instead of fixing stale docs. Fixed by excluding changelog-style files (CHANGELOG/CHANGES/HISTORY/ RELEASES.md, case-insensitive) from doc parsing entirely, the same way test files are excluded from code parsing. One fix is not re-validated end-to-end: the changelog exclusion is confirmed correct at the unit level (7 real-world filename cases, all correct) but re-running the full live pipeline to confirm it against actual API output hit the free tier's *daily* embedding quota (1000 requests/day, distinct from the per-minute limits the retry logic handles) - documented as a known limitation in TESTING.md rather than silently claimed as fully verified. --- TESTING.md | 35 ++++++++++++++++ packages/core/src/changes/filter.ts | 7 +--- packages/core/src/docs/file-walker.ts | 9 ++++- packages/core/src/docs/index.ts | 2 +- packages/core/src/index.ts | 1 + packages/core/src/linkgraph/embedding.ts | 5 ++- packages/core/src/llm/gemini-client.ts | 51 +++++++++++++++++------- packages/core/src/parsing/file-walker.ts | 11 +++-- packages/core/src/shared/is-test-file.ts | 5 +++ 9 files changed, 100 insertions(+), 26 deletions(-) create mode 100644 TESTING.md create mode 100644 packages/core/src/shared/is-test-file.ts diff --git a/TESTING.md b/TESTING.md new file mode 100644 index 0000000..17aaa74 --- /dev/null +++ b/TESTING.md @@ -0,0 +1,35 @@ +# 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. 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..35962ae 100644 --- a/packages/core/src/linkgraph/embedding.ts +++ b/packages/core/src/linkgraph/embedding.ts @@ -38,7 +38,10 @@ export async function buildEmbeddingLinks( 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. + 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..fbc5c9a 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,25 @@ 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; + +function isRetryable(error: unknown): boolean { + return error instanceof ApiError && (error.status === 429 || error.status === 503); +} + +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 +48,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 +68,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); +} From 53f2cdfbb282cb6fbf38b435de046d82a5e43ae1 Mon Sep 17 00:00:00 2001 From: dakshcodez Date: Fri, 14 Aug 2026 23:53:20 +0530 Subject: [PATCH 2/2] Fix issues found by subagent review of the real-repo testing PR - gemini-client.ts: withRetry only caught ApiError (HTTP-level 429/503) - a raw network failure (DNS, connection reset, timeout) never gets wrapped into ApiError by the SDK and was previously not retried at all, despite being exactly the kind of transient flakiness this fix was meant to cover, especially relevant since this runs in CI. Now also retries on ECONNRESET/ETIMEDOUT/ ECONNREFUSED/ENOTFOUND/EAI_AGAIN error codes, AbortError, and TypeError-with-cause (how fetch() itself reports network failure), while still throwing immediately for anything else. - embedding.ts: switching insertItem to upsertItem in the previous commit fixed the crash on duplicate chunk ids, but traded a loud failure for a silent one - one of the two colliding chunks now just quietly becomes unlinkable with no indication anywhere. Added a console.warn on collision, so the fix stays non-fatal but the data loss is now observable instead of invisible. - TESTING.md: added a known-limitations note that the new isTestFile/ isChangelogFile exclusions are unconditional - .sealignore exists as a user-facing override but isn't wired into the indexing walk, only into check.ts's changed-file filtering (pre-existing gap, not introduced by this PR) - so there's currently no way to opt a specific file back in. Found by a fresh subagent review spawned after PR #8 was opened. Both code fixes verified against real behavior: four separate network- failure types (ECONNRESET, TypeError+cause, AbortError, and a control case confirming non-retryable errors still throw immediately) tested against the live GeminiClient with the underlying SDK call mocked out per-attempt; the duplicate-id warning verified firing with the exact expected message via a real buildLinkGraph call with two colliding chunks. --- TESTING.md | 1 + packages/core/src/linkgraph/embedding.ts | 7 +++++++ packages/core/src/llm/gemini-client.ts | 15 ++++++++++++++- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/TESTING.md b/TESTING.md index 17aaa74..c84560a 100644 --- a/TESTING.md +++ b/TESTING.md @@ -33,3 +33,4 @@ Also observed, not fixed: the correction pass occasionally touched unrelated for - 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/linkgraph/embedding.ts b/packages/core/src/linkgraph/embedding.ts index 35962ae..89f6c73 100644 --- a/packages/core/src/linkgraph/embedding.ts +++ b/packages/core/src/linkgraph/embedding.ts @@ -36,11 +36,18 @@ 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)); // 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 }); } diff --git a/packages/core/src/llm/gemini-client.ts b/packages/core/src/llm/gemini-client.ts index fbc5c9a..ef58e98 100644 --- a/packages/core/src/llm/gemini-client.ts +++ b/packages/core/src/llm/gemini-client.ts @@ -12,9 +12,22 @@ 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 { - return error instanceof ApiError && (error.status === 429 || error.status === 503); + 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 {