Add cook lsp sub command for language server

[dubadub]

Add a new cook lsp command that starts the Cooklang Language Server Protocol server. The LSP server provides IDE features for Cooklang recipe files including real-time syntax checking, auto-completion, semantic highlighting, hover documentation, and document symbols.

Uses the cooklang-language-server crate and communicates over stdin/stdout using the standard LSP protocol for editor integration.

[claude]

Code Review: Add cook lsp sub command for language server

Thank you for this contribution! This is a well-structured PR that adds LSP support to CookCLI. Here's my comprehensive review:

---

✅ Strengths

1. Clean Integration: The LSP command follows the existing command architecture pattern perfectly, with proper integration into args.rs, main.rs, and lib.rs.

2. Excellent Documentation: The docs/lsp.md file is comprehensive and includes:

   • Clear overview of features
   • Editor integration examples for 8+ editors (VS Code, Neovim, Vim, Emacs, Helix, Sublime, Zed)
   • Troubleshooting guidance
   • Protocol details

3. Dependencies: Uses the external cooklang-language-server crate (0.1.0) which promotes code reuse and separation of concerns.

4. Graceful Shutdown: Implements proper signal handling with tokio::select! for Ctrl+C shutdown.

5. Consistent with Project Style: Follows CLAUDE.md guidelines and UNIX philosophy.

---

🔍 Code Quality Observations

src/lsp.rs

Good:

• Proper error handling with anyhow::Result
• Clear documentation explaining the Context usage
• Debug logging for configuration paths
• Clean async implementation with #[tokio::main]

Minor Concerns:

1. Nested Tokio Runtime (Line 20): The #[tokio::main] attribute creates a nested runtime since the main function in main.rs doesn't use async. This works but is slightly unconventional.

   • Suggestion: Consider whether the LSP command truly needs #[tokio::main] or if it could use the runtime differently. However, since this pattern works and the tower-lsp crate requires an async context, this is acceptable.

2. Context Usage: The Context is logged but not currently used by the LSP server. The comment explains this is for future extensibility, which is good.

   • Suggestion: Consider whether the Context should be passed to Backend::new() for future use, even if unused now. This would make future extension easier.

3. Empty Args Struct (Line 9-11): LspArgs is empty with a trailing #[command()] attribute.

   • Minor: The empty #[command()] attribute can be removed since there are no specific configurations.

---

🐛 Potential Issues

Test Snapshot Updates

The PR includes unrelated test snapshot changes (recipes showing different unit formatting like cups → c, scalable: true → scalable: false). These appear to be from updates to the cooklang parser dependency.

Files affected:

• tests/snapshots/snapshot_test__recipe_human_output.snap
• tests/snapshots/snapshot_test__recipe_json_output.snap
• tests/snapshots/snapshot_test__recipe_markdown_output.snap
• tests/snapshots/snapshot_test__recipe_yaml_output.snap
• tests/snapshots/snapshot_test__scaled_recipe_output.snap

Questions:

1. Are these changes intentional from upgrading cooklang to 0.17.4?
2. Should this be mentioned in the PR description?
3. Do these changes affect recipe parsing behavior?

---

⚡ Performance Considerations

1. Binary Size: Adding tower-lsp, cooklang-language-server, and their transitive dependencies (dashmap, ropey, lsp-types, futures, pin-project, etc.) will increase binary size.

   • Consideration: Since LSP is a core developer feature, this is acceptable.

2. Runtime Overhead: The LSP command spawns an async runtime. This is necessary for the protocol and has negligible overhead.

---

🔒 Security Concerns

1. No Authentication: LSP communicates over stdin/stdout with no authentication. This is standard for LSP servers and acceptable since they're typically launched by trusted editors.

2. File System Access: The LSP server will have access to the workspace directory. This is expected behavior.

3. Dependency Trust: The cooklang-language-server crate is from the same organization (cooklang) and version 0.1.0 suggests it's new. Ensure it's properly vetted.

---

🧪 Test Coverage

Missing:

• No integration tests for the LSP command
• No tests verifying the command launches successfully
• No tests for error handling (e.g., when stdin/stdout are not available)

Suggestions:

1. Add a basic integration test that verifies cook lsp launches without errors
2. Consider testing graceful shutdown
3. Test that the command appears in cook --help output (already covered by existing snapshot test)

Note: The help output snapshot test was correctly updated to include the new LSP command.

---

📝 Documentation

Excellent:

• docs/lsp.md is comprehensive and well-structured
• Inline code comments explain the Context usage
• Help text in args.rs is clear and informative

Suggestions:

1. Update the main README.md to mention LSP support (if not already present)
2. Consider adding a link to docs/lsp.md from the main documentation

---

🎯 Best Practices Adherence

✅ Follows Rust idioms and conventions
✅ Uses appropriate error handling patterns
✅ Consistent with existing command structure
✅ Good use of logging (info, debug levels)
✅ Proper shutdown signal handling
✅ Clear separation of concerns

---

🔧 Recommendations

High Priority:

1. Clarify snapshot changes: Explain the recipe output changes in the PR description or split into a separate commit
2. Add basic integration test: Ensure the command can launch successfully

Medium Priority:

3. Consider Context usage: Plan how Context will be used in the future and whether to pass it to Backend now
4. Remove empty attribute: Clean up the empty #[command()] on LspArgs

Low Priority:

5. Update main README: Add mention of LSP support if not already present
6. Consider feature flag: If binary size is a concern, consider making LSP an optional feature

---

📊 Summary

Overall Assessment: ✅ Approve with minor suggestions

This is a solid implementation that adds valuable IDE functionality to CookCLI. The code is clean, well-documented, and follows project conventions. The main concerns are:

• Unexplained test snapshot changes
• Missing integration tests
• Minor code cleanup opportunities

The implementation is production-ready, and the suggestions above are mostly about polish and completeness rather than blocking issues.

Recommendation: Approve and merge after:

1. Clarifying the snapshot changes
2. Optionally adding a basic integration test

Great work! 🎉

[claude]

Code Review: Add cook lsp subcommand for language server

I've reviewed PR #241 and have the following feedback:

✅ Strengths

1. Clean Architecture: The implementation follows CookCLI's established patterns perfectly - command module in src/lsp.rs with LspArgs struct and run() function, proper integration in src/args.rs and src/main.rs, consistent with other command modules

2. Excellent Documentation: The docs/lsp.md file is comprehensive with clear feature descriptions, multiple editor integration examples (VS Code, Neovim, Vim, Emacs, Helix, Sublime, Zed), troubleshooting section, and protocol details

3. Simple Implementation: Correctly delegates to cooklang-language-server crate following Unix philosophy

4. Graceful Shutdown: Uses tokio::select! to handle both server completion and Ctrl+C signals properly

5. Good Logging: Debug logs for context paths provide visibility for troubleshooting

⚠️ Issues & Recommendations

1. Async Runtime Mixing (Medium Priority)

The #[tokio::main] macro on run() at line 20 of src/lsp.rs creates a nested async runtime since it's called from synchronous main(). While it may work currently, nested runtimes can cause subtle issues with runtime shutdown, task spawning, and resource cleanup.

Recommendation: Remove #[tokio::main] from lsp::run and in main.rs wrap the call in a runtime like: tokio::runtime::Runtime::new()?.block_on(lsp::run(&ctx, args))

Or check if server::run() uses #[tokio::main] and follow that pattern for consistency.

2. Test Coverage (Low Priority)

No tests for the LSP command. While this aligns with current project practices (CLAUDE.md notes no automated tests), consider adding at least a smoke test that verifies the command can be invoked without panicking, or an integration test ensuring the server starts and responds to an LSP initialize request. This would be valuable as LSP integration is complex and can break subtly.

3. Snapshot Test Updates (Minor)

The test snapshots show changes unrelated to the LSP feature (cups → c, scalable: true → scalable: false). These appear to be from upstream cooklang crate changes. Consider splitting these into a separate commit/PR to keep changes focused, though if inevitable due to dependency updates, it's acceptable.

4. Context Usage (Question)

The Context is logged but not actively used. The comment suggests future extensibility, but it's unclear if current functionality is missing. Verify that the LSP server correctly discovers aisle.conf and pantry.conf from workspace folders. If there's a way to pass the context paths to the backend, consider doing so.

🔒 Security Review

✅ No security concerns - stdin/stdout communication is standard for LSP, no user input validation needed (handled by tower-lsp), no network exposure or file system access beyond what the library handles

🚀 Performance

✅ Async runtime is appropriate for LSP's I/O-bound nature, delegating to cooklang-language-server keeps this lightweight, no blocking operations

📦 Dependencies

✅ All dependencies look appropriate: cooklang-language-server 0.1.0, tower-lsp 0.20 (standard LSP framework), and standard transitive deps

🎯 Code Quality

✅ Follows Rust naming conventions, proper error handling with anyhow::Result, clear comments, consistent with project style

✨ Overall Assessment

This is a high-quality PR that follows CookCLI's conventions well. The main issue is the nested async runtime which should be addressed before merging. The documentation is exceptional and will help users integrate the LSP server into their workflows. The feature itself is valuable and will significantly improve the developer experience for Cooklang recipe authors.

Recommendation: Request changes for the async runtime issue, then approve after that's resolved.