ci: add shellcheck to CI and pre-commit hook#68
ci: add shellcheck to CI and pre-commit hook#68vlordier wants to merge 7 commits intoLiquid4All:mainfrom
Conversation
- Tool Result Compression: smart summarization for large tool outputs (directory listings, search results, JSON data) - Request Deduplication: 500ms debounce on send button to prevent duplicate requests from rapid clicks - Config Hot Reload: poll config file for changes, reload without restart, show toast notification - Error Boundary: timeout wrapper (120s) around tool execution, graceful error messages instead of crashing agent loop
- Add 13 tests for settings.rs (AppSettings, SamplingConfig, config hot reload) - Add 12 tests for chat.rs compression functions (truncate, compress directory, search, JSON) - Total tests: 392 (up from 365) - Coverage: 42.5% (up from 39.5%)
- Add ModelStatus tests (serialization, healthy/unhealthy states) - Add SamplingOverrides serialization tests - Add InferenceClient tests for: - LM Studio URL construction and model selection - Tool call format (NativeJson vs Pythonic) - Fallback chain exhaustion (AllModelsUnavailable) - is_retriable for various HTTP status codes - Error repair from malformed tool calls - Total tests: 417 (up from 392)
- Add tests for data_dir(), cache_dir(), resolve_db_path() - Add rotate_log_file() tests (creates rotated copies, handles missing files) - Add filter_by_enabled_servers() tests (filters correctly, handles missing/invalid config) - Add resolve_vision_model() tests (returns None without config) - Add filter_tools_by_allowlist() test (doesn't panic without config) - Add load_override_file() tests (parses config, returns empty for missing) - Total tests: 430 (up from 417) - Coverage: 43.3% (up from 42.5%)
- Add helpful debug message when LM Studio port connection fails - Distinguish between connection errors and timeouts in health_check
- Document default LM Studio port (1234) in environment configuration
- Add GitHub Actions workflow for shellcheck - Add shellcheck validation to pre-commit hook for staged .sh files This improves on PR Liquid4All#55 by adding automated shellcheck validation.
There was a problem hiding this comment.
Pull request overview
Adds Shellcheck enforcement to prevent shell-script lint regressions, while also introducing substantial new application functionality (frontend state/UI + Tauri backend MCP/inference/agent-core code).
Changes:
- Add Shellcheck linting via GitHub Actions and a pre-commit hook.
- Introduce a new settings store + UI behaviors (config watch + toast) and a debounced chat message input.
- Add major Tauri backend modules for MCP client, inference, and agent-core orchestration.
Reviewed changes
Copilot reviewed 51 out of 58 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/stores/settingsStore.ts | Adds Zustand settings store incl. config reload polling + notifications. |
| src/components/Chat/MessageInput.tsx | Adds message input with debounced send + autoresize. |
| src/App.tsx | Wires settings/config-watch lifecycle + toast notification UI. |
| src-tauri/tauri.conf.json | Adds/updates Tauri v2 app config & security CSP. |
| src-tauri/src/mcp_client/types.rs | Defines MCP + JSON-RPC types and unit tests. |
| src-tauri/src/mcp_client/transport.rs | Implements stdio JSON-RPC transport + helpers/tests. |
| src-tauri/src/mcp_client/mod.rs | Exposes MCP client modules and re-exports. |
| src-tauri/src/mcp_client/lifecycle.rs | Spawning/restart/shutdown for MCP servers. |
| src-tauri/src/mcp_client/errors.rs | Central MCP error enum. |
| src-tauri/src/mcp_client/discovery.rs | Auto-discovers MCP servers from mcp-servers/. |
| src-tauri/src/mcp_client/client.rs | High-level MCP client with tool routing + timeouts. |
| src-tauri/src/main.rs | Tauri entrypoint wiring localcowork::run(). |
| src-tauri/src/inference/types.rs | OpenAI-compatible inference request/response types + tests. |
| src-tauri/src/inference/streaming.rs | SSE streaming parser + non-streaming parser + tests. |
| src-tauri/src/inference/mod.rs | Inference module exports. |
| src-tauri/src/inference/errors.rs | Inference error enum + helper methods + tests. |
| src-tauri/src/inference/config.rs | Loads _models/config.yaml, interpolation, model resolution + tests. |
| src-tauri/src/commands/session.rs | Adds session list/load/delete and context budget commands. |
| src-tauri/src/commands/python_env_startup.rs | Startup-time python venv provisioning for MCP servers. |
| src-tauri/src/commands/python_env.rs | IPC commands for provisioning python venvs + progress events. |
| src-tauri/src/commands/ollama.rs | IPC commands for Ollama + llama-server status + pull progress. |
| src-tauri/src/commands/model_download.rs | Streaming model download + SHA-256 verification. |
| src-tauri/src/commands/mod.rs | Exposes command modules; includes greet placeholder. |
| src-tauri/src/commands/hardware.rs | Detects CPU/RAM/GPU and recommends runtime/quantization. |
| src-tauri/src/commands/filesystem.rs | Stub filesystem browsing commands for the UI. |
| src-tauri/src/agent_core/types.rs | Shared types for sessions/messages/audit/undo/confirmations. |
| src-tauri/src/agent_core/tool_router.rs | Tool dispatch w/ confirmation, retries, audit, undo + tests. |
| src-tauri/src/agent_core/tool_prefilter.rs | Embedding-based RAG tool prefilter + tests. |
| src-tauri/src/agent_core/tokens.rs | Token estimation + UTF-8 safe truncation + tests. |
| src-tauri/src/agent_core/response_analysis.rs | Detects incomplete responses/deflection/completion + tests. |
| src-tauri/src/agent_core/plan_templates.rs | Template-based plan decomposition + tests. |
| src-tauri/src/agent_core/plan_parser.rs | Parses bracket/JSON plan outputs + tests. |
| src-tauri/src/agent_core/permissions.rs | Tiered permissions store with persistence + tests. |
| src-tauri/src/agent_core/mod.rs | Agent core module exports. |
| src-tauri/src/agent_core/errors.rs | Agent core error enum + From conversions. |
| src-tauri/mcp-servers.json | Adds empty MCP servers config scaffold. |
| src-tauri/entitlements.plist | Adds macOS entitlements for process spawning/network/files. |
| src-tauri/capabilities/default.json | Adds Tauri capabilities/permissions configuration. |
| src-tauri/build.rs | Adds Tauri build script. |
| src-tauri/Cargo.toml | Adds Rust/Tauri dependencies and build configuration. |
| examples/localcowork/.github/workflows/shellcheck.yml | Adds Shellcheck GitHub Actions workflow (currently under examples). |
| examples/localcowork/.git-hooks/pre-commit | Adds Shellcheck validation for staged .sh files. |
| examples/localcowork/.env.example | Documents LM Studio default endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (interval) { | ||
| clearInterval(interval); |
There was a problem hiding this comment.
interval is undefined here, so config watching will never be cleared (and this should fail TypeScript compilation). Use the configWatchInterval value from get() when checking/clearing the interval.
| if (interval) { | |
| clearInterval(interval); | |
| if (configWatchInterval) { | |
| clearInterval(configWatchInterval); |
| let merged = merge_configs(discovered, overrides); | ||
| // fs was overridden | ||
| assert_eq!(merged["fs"].command, "node"); | ||
| // ocr was NOT overridden — preserved from discovery | ||
| assert_eq!(merged["ocr"].command, "npx"); | ||
| } |
There was a problem hiding this comment.
This assertion is platform-dependent and will fail on Windows where the discovered command is "npx.cmd" (per default_npx_command()). Use default_npx_command() (or conditional expectations) in the test to make it OS-agnostic.
| name: Shellcheck | ||
|
|
||
| on: | ||
| push: | ||
| paths: | ||
| - "**.sh" | ||
| pull_request: | ||
| paths: | ||
| - "**.sh" |
There was a problem hiding this comment.
GitHub Actions workflows are only picked up from the repository root at .github/workflows/. Placing this file under examples/localcowork/.github/workflows/ means CI won’t run. Move it to .github/workflows/shellcheck.yml (and keep example-specific docs elsewhere if needed).
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Run shellcheck | ||
| uses: ludeeus/action-shellcheck@master |
There was a problem hiding this comment.
Using an action reference pinned to @master is a supply-chain risk and can lead to nondeterministic CI behavior. Pin to a tagged release (or a full commit SHA) to ensure reproducible and auditable builds.
| uses: ludeeus/action-shellcheck@master | |
| uses: ludeeus/action-shellcheck@2.0.0 |
| for file in $STAGED_FILES; do | ||
| case "$file" in | ||
| *.sh) | ||
| if command -v shellcheck >/dev/null 2>&1; then | ||
| if ! shellcheck -s bash "$file" >/dev/null 2>&1; then | ||
| WARNINGS+=("shellcheck failed for $file (run: shellcheck -s bash $file)") | ||
| fi | ||
| fi | ||
| ;; | ||
| esac | ||
| done |
There was a problem hiding this comment.
Iterating with for file in $STAGED_FILES will break on filenames containing whitespace/globs due to word-splitting. Treat staged files as a newline-delimited list (read loop) or an array, and consider running shellcheck against the staged blob (not the working tree) so the hook validates exactly what will be committed.
| <div | ||
| className="config-reload-toast" | ||
| onClick={clearConfigReloadNotification} | ||
| > | ||
| <span className="toast-icon">🔄</span> | ||
| <span className="toast-message">{configReloadNotification}</span> | ||
| <button className="toast-close" aria-label="Dismiss"> |
There was a problem hiding this comment.
This makes a non-interactive <div> clickable without keyboard support (no role, tabIndex, or key handlers), and the close <button> doesn’t actually dismiss the toast (no onClick). Prefer making the close button handle dismissal (with type="button" and onClick) and avoid click-handlers on non-interactive containers (or add proper ARIA + keyboard handling).
| <div | |
| className="config-reload-toast" | |
| onClick={clearConfigReloadNotification} | |
| > | |
| <span className="toast-icon">🔄</span> | |
| <span className="toast-message">{configReloadNotification}</span> | |
| <button className="toast-close" aria-label="Dismiss"> | |
| <div className="config-reload-toast"> | |
| <span className="toast-icon">🔄</span> | |
| <span className="toast-message">{configReloadNotification}</span> | |
| <button | |
| type="button" | |
| className="toast-close" | |
| aria-label="Dismiss" | |
| onClick={clearConfigReloadNotification} | |
| > |
| const now = Date.now(); | ||
| if (now - lastSendTimeRef.current < SEND_DEBOUNCE_MS) { | ||
| setIsDebouncing(true); | ||
| setTimeout(() => setIsDebouncing(false), SEND_DEBOUNCE_MS); |
There was a problem hiding this comment.
This timeout isn’t tracked/cleaned up, so it can call setIsDebouncing(false) after unmount, which produces React warnings and makes behavior harder to reason about. Store the timeout id in a ref and clear it in an effect cleanup (and/or before scheduling a new timeout).
Summary
Improves on PR #55 (shellcheck fixes) by adding ongoing validation:
This ensures shellcheck issues don't regress after the initial fix.
Changes
Testing