refactor: ponytail lean pass — delete dead FP layer (Either/Railway)#74
Merged
Conversation
Ultra-intensity, repo-scope Ponytail lean-coding pass. Climbs to rung 1 (delete/YAGNI) for the high-confidence cuts and leaves the rest as decisions. Applied (provably zero behaviour change): - Delete src/domain/functional/either.ts (316 LOC) and railway.ts (412 LOC): no non-test, non-barrel code consumed any Either or Railway symbol. Result and Option carry all real error/nullable handling and are kept. - Trim the functional barrel (index.ts) and README to Result + Option only. - Delete 5 stale .gitkeep placeholders in now-populated dirs. - Mark the deliberate ceiling with one ponytail: marker in index.ts. Not applied (left as deliberate decisions): - Two parallel exception hierarchies (crosses the error-handling safety floor). - WordNet single-production-strategy abstraction (owner judgement call). Verified: tsc -p tsconfig.build.json typecheck clean; 1207 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
epub2 was declared in dependencies but imported nowhere — the EPUB loader uses the separate epub package. Removing it drops a dead runtime dependency. Verified: tsc -p tsconfig.build.json typecheck clean; 1207 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The flat domain/exceptions.ts hierarchy declared 8 classes but only 3 are referenced anywhere: SchemaValidationError, InvalidEmbeddingsError, and ConceptNotFoundError (LanceDB read path). Delete the 4 with zero references: SourceNotFoundError, InvalidQueryError, DatabaseOperationError, and MissingParameterError. (The live InvalidQueryError is the one in domain/exceptions/search.ts, which is unaffected.) The 3 live classes are kept on the standalone DomainException base rather than merged into the ConceptRAGError hierarchy: that migration would change tool-boundary error formatting and error codes, and no test (unit or integration) covers these throw paths. A ponytail: marker records the ceiling and its upgrade trigger. Verified: tsc -p tsconfig.build.json typecheck clean; 1207 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolves the two-parallel-hierarchies issue properly, instead of leaving the legacy flat exceptions on a separate base. - Add domain/exceptions/data-integrity.ts: a DataIntegrityError category under ConceptRAGError for the read-path errors (stored data doesn't match expectations), with ConceptNotFoundError, InvalidEmbeddingsError, and SchemaValidationError. Constructor signatures, public properties, messages, and error codes are preserved verbatim, so the ~10 throw sites and the repo instanceof checks change only their import path. - Repoint the 3 importers (schema-validators + 2 LanceDB repos) at domain/exceptions/index.js and merge the duplicate same-module imports. - Delete the flat domain/exceptions.ts and its vestigial re-export from domain/models/index.ts (nothing imported exceptions via the models barrel). Behaviour change, made deliberate and now tested: these 3 errors are now instanceof ConceptRAGError, so BaseTool.handleError emits their structured form (code + context + timestamp) at the tool boundary instead of a bare message. Adds the coverage these throw paths never had: - domain/exceptions/__tests__/data-integrity.test.ts (props/codes/instanceof) - tools/base/__tests__/tool.test.ts (pins the structured boundary output) Verified: tsc -p tsconfig.build.json clean; unit suite 63 files / 1213 tests pass; error-handling.integration.test.ts (16 tests) passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Ponytail lean-coding pass (ultra · repo)
Drives the codebase toward its leanest form: climb to rung 1 (delete/YAGNI) for the high-confidence cuts, then properly refactor the one structural issue that a deletion couldn't fix — while holding the safety floor (esp. error handling).
Cumulative: 20 files, +219 / −1353. Four independently-reviewable commits.
Commits
refactor: ponytail lean pass — delete dead FP layereither.ts(316) +railway.ts(412); trim thefunctional/barrel + README toResult+Option; delete 5 stale.gitkeep. No code consumed anyEither/Railwaysymbol.chore(deps): drop unused epub2 dependencyepub2declared but imported nowhere (the EPUB loader usesepub).refactor: delete 4 dead classes from legacy exceptionsdomain/exceptions.ts.refactor: unify exception hierarchies under ConceptRAGErrorDataIntegrityErrorcategory underConceptRAGError; delete the flat file. One hierarchy.The exception unification (commit 4)
The two parallel hierarchies are now one. The 3 read-path errors (
ConceptNotFoundError,InvalidEmbeddingsError,SchemaValidationError) move intodomain/exceptions/data-integrity.tsas aDataIntegrityErrorcategory underConceptRAGError.instanceofchecks change only their import path.instanceof ConceptRAGError, soBaseTool.handleErroremits their structured form (code + context + timestamp) instead of a bare message — the consistency win. This is exactly what made a blind migration unsafe before; it's now pinned by tests.export * from '../exceptions.js'leak indomain/models/index.ts(nothing consumed exceptions via the models barrel).New test coverage for paths that previously had none:
domain/exceptions/__tests__/data-integrity.test.ts— props/codes/instanceof.tools/base/__tests__/tool.test.ts— pins the structured boundary output.Investigated and intentionally left alone
scripts/— documented operational tooling (CONTRIBUTING/FAQ/SETUP/TROUBLESHOOTING/USAGE, ADRs, a dedicateddiagnostics/README.md), not dead code.SynsetSelectionStrategy— has two real, separately-tested implementations + a publicsetStrategy()seam; meets the "second concrete case exists" bar, so the abstraction is earned.Checked and cleared:
EmbeddingProviderFactory(4 providers),DocumentLoaderFactory(multi-loader), the DI interfaces (real impls + test doubles),Result/Option.Verification
tsc -p tsconfig.build.jsontypecheck — clean (after each commit)vitest run, excl. service-dependent integration/e2e/ail) — 63 files, 1213 tests, all passerror-handling.integration.test.ts— 16 tests pass🤖 Generated with Claude Code