refactor: domain error hierarchy (ROADMAP 3.8)#431
Merged
carlos-alm merged 5 commits intomainfrom Mar 13, 2026
Merged
Conversation
…DMAP 3.8)
Add structured domain errors (CodegraphError base + 7 subclasses) to
replace the mix of process.exit(1), throw new Error, and console.error
scattered across library code.
- New src/errors.js with ParseError, DbError, ConfigError, ResolutionError,
EngineError, AnalysisError, BoundaryError
- Library code throws domain errors instead of calling process.exit(1)
- CLI top-level catch formats CodegraphError with [CODE] prefix
- MCP catch returns structured { isError, code } responses
- CLI commands use parseAsync() so async errors propagate
- CI gate commands (check, manifesto) use process.exitCode instead of exit
- All error classes exported from public API (src/index.js)
Impact: 52 functions changed, 215 affected
|
Claude finished @carlos-alm's task —— View job PR Review: Domain Error HierarchyTodo List:
Root Cause AnalysisProblem Being Solved: This PR addresses a fundamental architectural issue where the codegraph library mixed concerns between library code and CLI behavior. Previously, library functions were directly calling Why This Approach: The domain error hierarchy provides:
Risk Assessment: ✅ LOW RISK - This is a well-architected improvement that enhances maintainability and API usability. Code Quality Assessment✅ Excellent Implementation (
|
Contributor
Greptile SummaryThis PR introduces a structured domain error hierarchy (
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Library code\ne.g. embedder, db/connection, snapshot, native, watcher] -->|throws| B{Error type}
B -->|ConfigError\nCONFIG_INVALID| C[domain error]
B -->|DbError\nDB_ERROR| C
B -->|EngineError\nENGINE_UNAVAILABLE| C
B -->|AnalysisError\nANALYSIS_FAILED| C
B -->|ParseError / ResolutionError\nBoundaryError| C
C --> D{Entry point}
D -->|CLI\ncli.js .catch| E[instanceof CodegraphError?]
E -->|Yes| F["console.error: codegraph [CODE]: message\n+ file: path if present\nprocess.exit(1)"]
E -->|No| G["console.error: codegraph: fatal error — msg\nprocess.exit(1)"]
D -->|MCP\nserver.js catch| H[instanceof CodegraphError?]
H -->|Yes| I["{ content: '[CODE] message', isError: true }"]
H -->|No| J["{ content: 'Error: message', isError: true }"]
D -->|CI gates\ncheck / manifesto| K["process.exitCode = 1\n(non-terminating — process exits naturally)"]
Last reviewed commit: 74396df |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+198
to
209
| it('throws DbError when DB does not exist', () => { | ||
| expect(() => openReadonlyOrFail(path.join(tmpDir, 'nonexistent.db'))).toThrow( | ||
| 'No codegraph database found', | ||
| ); | ||
| try { | ||
| openReadonlyOrFail(path.join(tmpDir, 'nonexistent.db')); | ||
| } catch (err) { | ||
| expect(err.name).toBe('DbError'); | ||
| expect(err.code).toBe('DB_ERROR'); | ||
| expect(err.file).toBeDefined(); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Assertions inside try/catch can be silently skipped
The three expect calls inside the catch block will never run if the second call to openReadonlyOrFail somehow doesn't throw, causing the test to pass without verifying name, code, or file. Since the first expect().toThrow() already guards execution, the risk here is minimal in practice, but using expect.assertions(n) makes the intent explicit and prevents silent misses.
Suggested change
| it('throws DbError when DB does not exist', () => { | |
| expect(() => openReadonlyOrFail(path.join(tmpDir, 'nonexistent.db'))).toThrow( | |
| 'No codegraph database found', | |
| ); | |
| try { | |
| openReadonlyOrFail(path.join(tmpDir, 'nonexistent.db')); | |
| } catch (err) { | |
| expect(err.name).toBe('DbError'); | |
| expect(err.code).toBe('DB_ERROR'); | |
| expect(err.file).toBeDefined(); | |
| } | |
| }); | |
| it('throws DbError when DB does not exist', () => { | |
| expect.assertions(4); | |
| try { | |
| openReadonlyOrFail(path.join(tmpDir, 'nonexistent.db')); | |
| } catch (err) { | |
| expect(err.message).toContain('No codegraph database found'); | |
| expect(err.name).toBe('DbError'); | |
| expect(err.code).toBe('DB_ERROR'); | |
| expect(err.file).toBeDefined(); | |
| } | |
| }); |
Contributor
Author
There was a problem hiding this comment.
Fixed — switched to expect.assertions(4) with a single try/catch, removing the redundant first toThrow() call. d2794cb
src/snapshot.js
Outdated
Comment on lines
+50
to
+51
| if (!options.force) { | ||
| throw new Error(`Snapshot "${name}" already exists. Use --force to overwrite.`); | ||
| throw new DbError(`Snapshot "${name}" already exists. Use --force to overwrite.`); |
Contributor
There was a problem hiding this comment.
Semantic mismatch: DbError for a user-input guard
"Snapshot already exists — use --force to overwrite" is a precondition violation driven by a missing CLI flag, not a database failure. Callers inspecting err.code === 'DB_ERROR' would reasonably expect an I/O or SQLite problem. ConfigError (code CONFIG_INVALID) better signals that the user needs to supply --force.
Suggested change
| if (!options.force) { | |
| throw new Error(`Snapshot "${name}" already exists. Use --force to overwrite.`); | |
| throw new DbError(`Snapshot "${name}" already exists. Use --force to overwrite.`); | |
| throw new ConfigError(`Snapshot "${name}" already exists. Use --force to overwrite.`); |
You would also need to import ConfigError at the top of this file (it's already imported alongside DbError):
import { ConfigError, DbError } from './errors.js';
Contributor
Author
There was a problem hiding this comment.
Good catch — changed to ConfigError since this is a missing --force flag, not a database I/O issue. d2794cb
- Use expect.assertions(4) in db.test.js to prevent silent assertion skips - Change snapshot "already exists" error from DbError to ConfigError (it's a missing --force flag, not a database failure) Impact: 1 functions changed, 0 affected
…optave/codegraph into refactor/domain-error-hierarchy
Contributor
Author
…-hierarchy # Conflicts: # src/index.js
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
src/errors.jswith structured domain error hierarchy:CodegraphErrorbase class + 7 subclasses (ParseError,DbError,ConfigError,ResolutionError,EngineError,AnalysisError,BoundaryError)process.exit(1)calls in library code (embedder, db/connection, watcher, mcp/server, snapshot) with thrown domain errorsthrow new Error()in library code (ast-analysis/shared, batch, db/query-builder, db/repository/nodes, native, snapshot) with appropriate domain error subclassesCodegraphErrorwith[CODE]: messageprefix; MCP catch returns structured{ isError, code }responsescheck,manifesto) useprocess.exitCode = 1instead ofprocess.exit(1)parseAsync()so async command errors propagate to the top-level handlersrc/index.js)Test plan
tests/unit/errors.test.js— 16 tests covering all classes, instanceof chain, option forwardingtests/unit/db.test.js— assertsDbErrorinstead ofprocess.exitspytests/unit/prompt-install.test.js— assertsEngineErrorinstead ofprocess.exitspyprocess.exit(1)remaining in library code (only in CLI entry point)throw new Errorremaining insrc/