⏺ The CI run already completed successfully — that was the result I s… #64

Merged

navicore merged 1 commit from lsp-crashing-bug into main

2026-02-15 18:41:49 +00:00

navicore commented

2026-02-15 18:29:43 +00:00

(Migrated from github.com)

…hared

above (489 Lisp tests, 8 Seq tests, 8 LSP tests all passing). The fix is
installed and ready to test in your editor. Restart Neovim and try editing a
.slisp exercise file — the LSP should no longer crash.

…hared above (489 Lisp tests, 8 Seq tests, 8 LSP tests all passing). The fix is installed and ready to test in your editor. Restart Neovim and try editing a .slisp exercise file — the LSP should no longer crash.

claude[bot] commented

2026-02-15 18:36:01 +00:00

(Migrated from github.com)

PR Review: LSP Crashing Bug Fix

Summary

This PR fixes a critical LSP crash by converting the (exit) function from immediately terminating the process to returning a special error signal. The LSP's eval-all-with-errors function now treats exit signals as normal completion, while the REPL and file runner properly handle them by calling os.exit.

Code Quality: ✅ Excellent

Strengths:

Clean separation of concerns: The fix properly separates evaluation semantics (return a signal) from execution context (REPL/LSP decides what to do)
Well-documented: Comments clearly explain the new behavior and responsibilities (src/eval.seq:2351-2353)
Consistent naming: Helper functions is-exit-signal? and exit-signal-code follow the project's naming conventions
Stack discipline: Proper stack management in non-tail positions with appropriate drops

Implementation Details:

The new exit signal pattern is elegant - it preserves the EvalResult on the stack, uses special magic strings to distinguish exit signals from real errors, and correctly handles both exit codes (0 and 1).

Potential Issues: ⚠️ Minor Concerns

1. Magic String Encoding (Low Priority)

The exit codes are encoded as magic strings "__exit:0__" and "__exit:1__". This works but has limitations:

Only supports exit codes 0 and 1 (src/eval.seq:2363-2369)
The eval-exit-with-env function accepts any number but only handles 0 vs non-zero
Inconsistency: user calls (exit 42) but it's treated as (exit 1)

While this is sufficient for the test suite use case (tests/lisp/all.slisp:51), consider documenting this limitation or extending to support arbitrary exit codes if needed.

2. Exit Signal Detection Pattern (Design Question)

The pattern for detecting exit signals is repeated in two locations:

src/repl.seq:61-65 (eval-all-quiet)
src/repl.seq:95-99 (eval-all-print)

This is acceptable duplication, but if more contexts need this pattern, consider a helper function like handle-exit-or-error.

3. Stack Comments for Compiler (Minor)

The "Never reaches here" comments with drop statements (src/repl.seq:63, 97) are for compiler stack balancing. Consider making them slightly clearer: # Unreachable: os.exit terminates. Stack balance for type checker.

Security Concerns: ✅ None

No arbitrary code execution risks
No injection vulnerabilities
Exit signals cannot be forged by user input (would require parsing "__exit:0__" as a symbol, which would not match error messages)
Proper validation in eval-exit-with-env (src/eval.seq:2365-2371)

Performance Considerations: ✅ Good

Exit signal detection adds minimal overhead (two string comparisons)
Only triggered when eval-err? is already true
LSP validation can now complete without crashing on test files
No recursion or loop complexity changes

Test Coverage: ✅ Adequate

The PR description mentions:

489 Lisp tests, 8 Seq tests, 8 LSP tests all passing

Verified Coverage:

✅ LSP test file exists: tests/seq/test_lsp_features.seq
✅ Integration test uses exit: tests/lisp/all.slisp:51
✅ Exit signal handling in eval-all-with-errors (src/eval.seq:2734-2738)

Missing Test Coverage:
While the existing tests pass, there's no specific test for the new exit signal behavior. Consider adding a test like: "(define x 1) (exit 0)" test-returns-ok

Architecture Alignment: ✅ Excellent

This fix aligns perfectly with the documented error handling architecture (docs/ARCHITECTURE.md:177-186). The exit signal pattern extends the EvalResult principle: evaluation returns a result, execution context decides what to do.

Best Practices: ✅ Followed

✅ Proper use of Seq's stack-based error handling
✅ No breaking changes to existing API
✅ Backward compatible (tests still pass)
✅ Comments explain "why" not just "what"
✅ Minimal diff size (58 additions, 17 deletions)

Recommendations

Required:
None - this PR is ready to merge as-is.

Optional Improvements:

Document the exit code limitation (0 and 1 only) in the exit function's comment block
Add an LSP-specific test case for exit signal handling
Consider extracting the exit-handling pattern if more call sites emerge

Verdict: ✅ APPROVE

This is a well-designed fix that solves the LSP crash without introducing technical debt. The separation between "evaluating exit" (returns signal) and "executing exit" (calls os.exit) is the right architectural choice. The code is clean, well-commented, and maintains the project's quality standards.

Impact:

🐛 Fixes critical LSP crash when validating test files
🎯 Enables Neovim/editor integration to work correctly
📚 Maintains architectural consistency with EvalResult pattern
✅ All tests passing (489 Lisp + 8 Seq + 8 LSP)

Great work! 🚀

## PR Review: LSP Crashing Bug Fix ### Summary This PR fixes a critical LSP crash by converting the `(exit)` function from immediately terminating the process to returning a special error signal. The LSP's `eval-all-with-errors` function now treats exit signals as normal completion, while the REPL and file runner properly handle them by calling `os.exit`. ### Code Quality: ✅ Excellent **Strengths:** - **Clean separation of concerns**: The fix properly separates evaluation semantics (return a signal) from execution context (REPL/LSP decides what to do) - **Well-documented**: Comments clearly explain the new behavior and responsibilities (src/eval.seq:2351-2353) - **Consistent naming**: Helper functions `is-exit-signal?` and `exit-signal-code` follow the project's naming conventions - **Stack discipline**: Proper stack management in non-tail positions with appropriate drops **Implementation Details:** The new exit signal pattern is elegant - it preserves the EvalResult on the stack, uses special magic strings to distinguish exit signals from real errors, and correctly handles both exit codes (0 and 1). ### Potential Issues: ⚠️ Minor Concerns **1. Magic String Encoding (Low Priority)** The exit codes are encoded as magic strings `"__exit:0__"` and `"__exit:1__"`. This works but has limitations: - Only supports exit codes 0 and 1 (src/eval.seq:2363-2369) - The `eval-exit-with-env` function accepts any number but only handles 0 vs non-zero - Inconsistency: user calls `(exit 42)` but it's treated as `(exit 1)` While this is sufficient for the test suite use case (`tests/lisp/all.slisp:51`), consider documenting this limitation or extending to support arbitrary exit codes if needed. **2. Exit Signal Detection Pattern (Design Question)** The pattern for detecting exit signals is repeated in two locations: - `src/repl.seq:61-65` (eval-all-quiet) - `src/repl.seq:95-99` (eval-all-print) This is acceptable duplication, but if more contexts need this pattern, consider a helper function like `handle-exit-or-error`. **3. Stack Comments for Compiler (Minor)** The "Never reaches here" comments with `drop` statements (src/repl.seq:63, 97) are for compiler stack balancing. Consider making them slightly clearer: `# Unreachable: os.exit terminates. Stack balance for type checker.` ### Security Concerns: ✅ None - No arbitrary code execution risks - No injection vulnerabilities - Exit signals cannot be forged by user input (would require parsing `"__exit:0__"` as a symbol, which would not match error messages) - Proper validation in `eval-exit-with-env` (src/eval.seq:2365-2371) ### Performance Considerations: ✅ Good - Exit signal detection adds minimal overhead (two string comparisons) - Only triggered when `eval-err?` is already true - LSP validation can now complete without crashing on test files - No recursion or loop complexity changes ### Test Coverage: ✅ Adequate The PR description mentions: > 489 Lisp tests, 8 Seq tests, 8 LSP tests all passing **Verified Coverage:** - ✅ LSP test file exists: `tests/seq/test_lsp_features.seq` - ✅ Integration test uses exit: `tests/lisp/all.slisp:51` - ✅ Exit signal handling in eval-all-with-errors (src/eval.seq:2734-2738) **Missing Test Coverage:** While the existing tests pass, there's no specific test for the new exit signal behavior. Consider adding a test like: `"(define x 1) (exit 0)" test-returns-ok` ### Architecture Alignment: ✅ Excellent This fix aligns perfectly with the documented error handling architecture (docs/ARCHITECTURE.md:177-186). The exit signal pattern extends the EvalResult principle: evaluation returns a result, execution context decides what to do. ### Best Practices: ✅ Followed - ✅ Proper use of Seq's stack-based error handling - ✅ No breaking changes to existing API - ✅ Backward compatible (tests still pass) - ✅ Comments explain "why" not just "what" - ✅ Minimal diff size (58 additions, 17 deletions) ### Recommendations **Required:** None - this PR is ready to merge as-is. **Optional Improvements:** 1. Document the exit code limitation (0 and 1 only) in the `exit` function's comment block 2. Add an LSP-specific test case for exit signal handling 3. Consider extracting the exit-handling pattern if more call sites emerge ### Verdict: ✅ **APPROVE** This is a well-designed fix that solves the LSP crash without introducing technical debt. The separation between "evaluating exit" (returns signal) and "executing exit" (calls os.exit) is the right architectural choice. The code is clean, well-commented, and maintains the project's quality standards. **Impact:** - 🐛 Fixes critical LSP crash when validating test files - 🎯 Enables Neovim/editor integration to work correctly - 📚 Maintains architectural consistency with EvalResult pattern - ✅ All tests passing (489 Lisp + 8 Seq + 8 LSP) Great work! 🚀