Phase 8: Real-world accuracy test against tj/commander.js - #8
Merged
Conversation
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.
- 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements Phase 5 of the original plan: a real end-to-end accuracy test against a real, live-forked repository, using the actual Gemini API (not mocks). Full methodology and results in
TESTING.md.Target: 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). Ranseal indexandseal checkfor real.Deliberate test results:
.name()→.setName())Three real bugs found and fixed, none of which mocked testing had caught:
GeminiClient.CHANGELOG.mdentries describing historical releases were getting "corrected" to match current code - rewriting history instead of fixing stale docs. Changelog-style files are now excluded from doc parsing entirely.Known limitation
The changelog-exclusion fix is unit-verified (7 real filename cases, all correct) but not re-validated end-to-end against live API output - re-running the full pipeline to confirm hit the free tier's daily embedding quota (1000/day, distinct from the per-minute limits the retry logic handles). Documented honestly in
TESTING.mdrather than claimed as fully verified.Test plan
npm run lint/typecheck/buildall passseal indexrun againsttj/commander.js(232 chunks, 365 sections, 194 links)seal checkrun with three deliberate code changes, verdicts and corrections inspected by handisChangelogFileunit-verified against 7 real-world filename cases