Analyze codebase for refactoring opportunities#27
Merged
Conversation
REFACTORING ANALYSIS: - Full codebase analysis identifying 8 major patterns of code duplication - ~800 lines of duplicated code identified across 7 scripts - Detailed non-breaking refactoring recommendations - Priority matrix for implementation - Backward compatibility guarantees documented NEW SHARED LIBRARY (lyrebird-common.sh v1.0.0): - Terminal color initialization with tput/ANSI fallback - Cached command_exists() for improved performance - Portable compute_hash() with sha256/shasum/cksum fallback - Unified logging functions (log_debug/info/warn/error) - Standard exit codes (E_SUCCESS, E_GENERAL, etc.) - File operations (is_readable, get_file_size) - Numeric validation (safe_base10, is_positive_integer) - Timeout wrapper (run_with_timeout) BACKWARD COMPATIBILITY: - All functions check if already defined before defining - Scripts work identically whether library exists or not - No changes required to existing deployments - All 8 scripts pass bash syntax validation
Adds conditional sourcing of lyrebird-common.sh to all 7 scripts: - mediamtx-stream-manager.sh - lyrebird-diagnostics.sh - lyrebird-updater.sh - lyrebird-orchestrator.sh - install_mediamtx.sh - lyrebird-mic-check.sh - usb-audio-mapper.sh INTEGRATION APPROACH: - Library sourced after set commands, before script constants - Uses conditional sourcing: [[ -f lib ]] && source lib || true - Falls back gracefully if library not present - Scripts retain all local function definitions as fallback - Zero changes to external API or behavior TESTING PERFORMED: - All 8 scripts pass bash syntax validation - Library loads correctly and provides all functions - command_exists, compute_hash, logging functions verified - Backward compatibility verified (scripts work without library) - Function override guards verified (pre-defined functions preserved) - Debug mode verified - Script help commands verified working BACKWARD COMPATIBILITY: - 100% compatible with existing deployments - No breaking changes to any script - Library is optional - scripts work identically without it - All existing configurations continue to work
lyrebird-orchestrator.sh: - SC2155: Separate declare and assign for export_file variable usb-audio-mapper.sh: - SC2155: Separate declare and assign for SCRIPT_NAME and SCRIPT_DIR (functionally identical - same values, same readonly status) - SC2034: Add shellcheck disable comment for SCRIPT_DIR - SC2034: Add shellcheck disable comment for usb_path parameter (NO rename - only added comment above existing line) All changes are cosmetic (comments and line splitting). Zero behavioral changes. Zero variable renames. All 8 scripts pass: - bash -n syntax validation - shellcheck with 0 errors and 0 warnings
Creates .github/workflows/bash-ci.yml with: Required checks (must pass to merge): - bash -n: Syntax validation for all shell scripts - shellcheck: Static analysis (errors and warnings) Advisory checks (informational only): - shfmt: Format consistency checking - bashate: OpenStack style guidelines - Security scan: Basic pattern detection Features: - Runs on PRs targeting main/master branches - Only triggers when .sh files change - Parallel job execution for speed - Detailed summary reports in GitHub UI - Concurrency control (cancels in-progress runs) All tools are free, open source, and require no API keys, subscriptions, or SaaS setup.
- Replace ((var++)) with var=$((var + 1)) to avoid exit code 1 when incrementing from 0 (bash arithmetic quirk with set -e) - Use while/read loops instead of for loops to handle filenames with spaces correctly - Add additional bashate ignores for noisy rules: - E001: Trailing whitespace (handled by shfmt) - E042: local declaration hides errors (valid bash pattern) - E043: Arithmetic compound semantics (valid bash patterns) Verified with actionlint and YAML syntax validation.
Security Scan Improvements: - Refined eval detection to exclude safe patterns: - 'eval set --' (standard getopt) - 'yq eval', 'jq eval' (tool commands) - Comments mentioning eval - Removed false-positive-prone unquoted variable check (ShellCheck handles this better) - Added new checks: - Unquoted command substitutions in rm/mv/cp - Predictable temp file patterns (/tmp/name.ext) - Improved hardcoded secrets detection (only actual values) - All patterns now exclude comments CI Summary Enhancements: - Comprehensive markdown report with sections - Overall status banner (passed/failed) - Results table with check type indicators (Required/Advisory) - Tool versions and links to documentation - Detailed explanation of what each check does - Collapsible quick-fix guide with commands - Script count in footer - Collapsible legend for check types Both security scan and summary now generate detailed GitHub Step Summary reports for easy PR review.
Formatting changes applied with: shfmt -w -i 4 -bn -ci Changes are purely cosmetic: - Remove trailing whitespace from blank lines - Standardize redirect operator spacing (>> → >>) - Add spaces in arithmetic expressions ((i=0 → (i = 0) - Normalize comment spacing (double space → single) - Binary operators at start of continuation lines Verified: - All 8 scripts pass bash -n syntax validation - All 8 scripts pass shellcheck (0 errors, 0 warnings) - No function signature changes - No logic or behavioral changes - Line count: 17940 → 17937 (only whitespace removed) This eliminates the shfmt CI advisory warning.
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
2 participants
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.
No description provided.