From 3bcf746c9f8d00a5884888068fe9c795884e924f Mon Sep 17 00:00:00 2001 From: Max Rantil Date: Tue, 4 Nov 2025 09:23:10 +0100 Subject: [PATCH 1/2] refactor: eliminate test duplication in workflow (resolves #60) Replaced ~240 lines of duplicated test logic in GitHub Actions workflow with a single call to the standalone test script. Changes: - Removed inline test steps from installation-test job - Now invokes tests/installation-test.sh directly - Maintains same functionality: 9 test scenarios, diagnostics on failure - Reduces workflow from ~240 lines to ~26 lines for the test job Benefits: - Single source of truth for test logic - Easier maintenance (update tests in one place) - Guaranteed consistency between CI and local testing Note: Pre-existing Docker test failure (Starship cache) is unrelated to this refactoring and also fails on master branch. --- .github/workflows/shell-quality.yml | 225 +--------------------------- 1 file changed, 2 insertions(+), 223 deletions(-) diff --git a/.github/workflows/shell-quality.yml b/.github/workflows/shell-quality.yml index 9e29043..cdfb9fa 100644 --- a/.github/workflows/shell-quality.yml +++ b/.github/workflows/shell-quality.yml @@ -61,230 +61,9 @@ jobs: TEMP_HOME=$(mktemp -d) echo "TEMP_HOME=$TEMP_HOME" >> $GITHUB_ENV echo "DIAGNOSTICS_DIR=$TEMP_HOME/diagnostics" >> $GITHUB_ENV - mkdir -p "$TEMP_HOME/diagnostics" - - name: Run install.sh in test home (first run) - id: first_run - run: | - # Environment isolation: Unset variables that could affect installation behavior - unset XDG_CONFIG_HOME XDG_DATA_HOME XDG_CACHE_HOME ZDOTDIR - export HOME=$TEMP_HOME - START_TIME=$(date +%s%3N) - bash install.sh 2>&1 | tee $DIAGNOSTICS_DIR/install-first-run.log - END_TIME=$(date +%s%3N) - DURATION=$((END_TIME - START_TIME)) - echo "first_run_duration=$DURATION" >> $GITHUB_OUTPUT - echo "First run took ${DURATION}ms" - env: - CI: true - - - name: Verify key symlinks created - id: verify_symlinks - run: | - # CRITICAL SYMLINKS TESTED IN CI: - # These are essential for basic shell functionality: - # - .zshrc: Shell initialization in ZDOTDIR (required for zsh to function) - # - .zprofile: Shell profile (environment setup, sourced by zsh) - # - .aliases: Core command shortcuts (workflow dependency) - # - nvim/init.vim: Editor configuration (development dependency) - # - .tmux.conf: Session management (SSH workflow requirement) - # - starship.toml: Prompt configuration (startup validation) - # - # CONDITIONAL SYMLINKS (tested if present): - # - .gitconfig: User-specific, conditional linking - # - inputrc: Readline enhancement, non-critical - # - # MAINTENANCE: When adding new critical symlinks to install.sh, - # update the test cases below to verify them. - # Last updated: 2025-11-03 (6 critical symlinks + conditional) - - # Check critical symlinks exist - ERRORS=0 - - # ZDOTDIR is set to $HOME/.config/zsh per .zprofile - ZDOTDIR="$TEMP_HOME/.config/zsh" - - echo "Verifying critical symlinks..." - - # Check .zshrc in ZDOTDIR - if [ ! -L "$ZDOTDIR/.zshrc" ]; then - echo "ERROR: .zshrc not linked in ZDOTDIR" | tee -a $DIAGNOSTICS_DIR/symlink-errors.log - ERRORS=$((ERRORS + 1)) - else - echo "✓ .zshrc (in ZDOTDIR)" - fi - - # Check other critical symlinks - for link in ".zprofile" ".aliases" ".config/nvim/init.vim" ".tmux.conf" ".config/starship.toml"; do - if [ ! -L "$TEMP_HOME/$link" ]; then - echo "ERROR: $link not linked" | tee -a $DIAGNOSTICS_DIR/symlink-errors.log - ERRORS=$((ERRORS + 1)) - else - echo "✓ $link" - fi - done - - # Check conditional symlinks if source exists - echo "Verifying conditional symlinks..." - if [ -f "$GITHUB_WORKSPACE/.gitconfig" ]; then - if [ ! -L "$TEMP_HOME/.gitconfig" ]; then - echo "ERROR: .gitconfig not linked (source exists)" | tee -a $DIAGNOSTICS_DIR/symlink-errors.log - ERRORS=$((ERRORS + 1)) - else - echo "✓ .gitconfig" - fi - fi - - if [ -f "$GITHUB_WORKSPACE/inputrc" ]; then - if [ ! -L "$TEMP_HOME/.config/shell/inputrc" ]; then - echo "ERROR: inputrc not linked (source exists)" | tee -a $DIAGNOSTICS_DIR/symlink-errors.log - ERRORS=$((ERRORS + 1)) - else - echo "✓ inputrc" - fi - fi - - if [ $ERRORS -gt 0 ]; then - exit 1 - fi - - - name: Verify symlink targets - id: verify_targets - run: | - echo "Verifying symlink targets point to correct files..." - ERRORS=0 - - verify_target() { - local symlink="$1" - local expected_target="$2" - - if [ -L "$TEMP_HOME/$symlink" ]; then - actual_target=$(readlink -f "$TEMP_HOME/$symlink") - if [ "$actual_target" != "$expected_target" ]; then - echo "ERROR: $symlink points to $actual_target, expected $expected_target" | tee -a $DIAGNOSTICS_DIR/target-errors.log - return 1 - else - echo "✓ $symlink -> $actual_target" - fi - fi - } - - verify_target ".zprofile" "$GITHUB_WORKSPACE/.zprofile" || ERRORS=$((ERRORS + 1)) - verify_target ".aliases" "$GITHUB_WORKSPACE/.aliases" || ERRORS=$((ERRORS + 1)) - verify_target ".config/nvim/init.vim" "$GITHUB_WORKSPACE/init.vim" || ERRORS=$((ERRORS + 1)) - verify_target ".tmux.conf" "$GITHUB_WORKSPACE/.tmux.conf" || ERRORS=$((ERRORS + 1)) - verify_target ".config/starship.toml" "$GITHUB_WORKSPACE/starship.toml" || ERRORS=$((ERRORS + 1)) - - if [ $ERRORS -gt 0 ]; then - exit 1 - fi - - - name: Check for broken symlinks - id: check_broken - run: | - echo "Checking for broken symlinks..." - cd $TEMP_HOME - if find . -type l ! -exec test -e {} \; -print | grep -q .; then - echo "ERROR: Found broken symlinks:" | tee $DIAGNOSTICS_DIR/broken-symlinks.log - find . -type l ! -exec test -e {} \; -print | tee -a $DIAGNOSTICS_DIR/broken-symlinks.log - exit 1 - fi - echo "✓ No broken symlinks found" - - - name: Test idempotency (second run) - id: second_run - run: | - echo "Testing idempotency - running install.sh again..." - unset XDG_CONFIG_HOME XDG_DATA_HOME XDG_CACHE_HOME ZDOTDIR - export HOME=$TEMP_HOME - START_TIME=$(date +%s%3N) - bash install.sh 2>&1 | tee $DIAGNOSTICS_DIR/install-second-run.log - END_TIME=$(date +%s%3N) - DURATION=$((END_TIME - START_TIME)) - echo "second_run_duration=$DURATION" >> $GITHUB_OUTPUT - echo "Second run took ${DURATION}ms" - - - name: Verify backup functionality - id: verify_backup - run: | - echo "Verifying backup functionality..." - - ZDOTDIR="$TEMP_HOME/.config/zsh" - - # Create a real file (not symlink) that should be backed up - echo "test content" > "$TEMP_HOME/.test-backup-file" - - # Run install.sh again - this should create backup if conflicts exist - unset XDG_CONFIG_HOME XDG_DATA_HOME XDG_CACHE_HOME ZDOTDIR - export HOME=$TEMP_HOME - bash install.sh 2>&1 | tee $DIAGNOSTICS_DIR/install-backup-test.log - - # Reset ZDOTDIR for verification checks - ZDOTDIR="$TEMP_HOME/.config/zsh" - - # Check if backup directory was created (may not be if no conflicts) - BACKUP_DIR=$(find $TEMP_HOME -maxdepth 1 -name ".dotfiles_backup_*" -type d | head -1) - - if [ -n "$BACKUP_DIR" ]; then - echo "✓ Backup directory created: $BACKUP_DIR" - else - echo "⚠ No backup directory created (no conflicts detected)" - fi - - # Verify backup is not created for symlinks (should replace them silently) - if [ -L "$ZDOTDIR/.zshrc" ]; then - echo "✓ Symlinks replaced correctly without backup" - else - echo "ERROR: .zshrc is not a symlink after re-installation" | tee -a $DIAGNOSTICS_DIR/backup-errors.log - exit 1 - fi - - - name: Verify idempotency (no duplicates) - id: verify_idempotency - run: | - echo "Verifying no duplicate operations occurred..." - - ZDOTDIR="$TEMP_HOME/.config/zsh" - - # Check that symlinks still point to correct locations - test -L "$ZDOTDIR/.zshrc" || (echo "ERROR: .zshrc removed during second run" && exit 1) - test -L "$TEMP_HOME/.aliases" || (echo "ERROR: .aliases removed during second run" && exit 1) - - # Verify no duplicate directories created - CONFIG_DIRS=$(find $TEMP_HOME/.config -type d -name "nvim" | wc -l) - if [ $CONFIG_DIRS -ne 1 ]; then - echo "ERROR: Duplicate directories created" | tee $DIAGNOSTICS_DIR/idempotency-errors.log - exit 1 - fi - - echo "✓ Installation is idempotent" - - - name: Performance regression check - id: perf_check - run: | - FIRST_RUN=${{ steps.first_run.outputs.first_run_duration }} - SECOND_RUN=${{ steps.second_run.outputs.second_run_duration }} - - echo "Performance metrics:" - echo " First run: ${FIRST_RUN}ms" - echo " Second run: ${SECOND_RUN}ms" - - # Set baseline threshold (30 seconds = 30000ms) - THRESHOLD=30000 - - if [ $FIRST_RUN -gt $THRESHOLD ]; then - echo "WARNING: First run exceeded ${THRESHOLD}ms threshold" | tee $DIAGNOSTICS_DIR/performance-warning.log - fi - - if [ $SECOND_RUN -gt $THRESHOLD ]; then - echo "WARNING: Second run exceeded ${THRESHOLD}ms threshold" | tee -a $DIAGNOSTICS_DIR/performance-warning.log - fi - - # Save metrics for historical tracking - echo "first_run_ms=$FIRST_RUN" >> $DIAGNOSTICS_DIR/performance-metrics.txt - echo "second_run_ms=$SECOND_RUN" >> $DIAGNOSTICS_DIR/performance-metrics.txt - - echo "✓ Performance check complete" + - name: Run comprehensive installation tests + run: ./tests/installation-test.sh "$TEMP_HOME" "$GITHUB_WORKSPACE" - name: Upload diagnostic artifacts if: failure() From a268f1a2ccc2773a2fc7cf3c6a66cb835ff18a91 Mon Sep 17 00:00:00 2001 From: Max Rantil Date: Tue, 4 Nov 2025 09:31:58 +0100 Subject: [PATCH 2/2] docs: update session handoff for issue #60 completion --- SESSION_HANDOVER.md | 129 ++++++++++++++++++++++++++++---------------- 1 file changed, 83 insertions(+), 46 deletions(-) diff --git a/SESSION_HANDOVER.md b/SESSION_HANDOVER.md index 41a16c4..e456469 100644 --- a/SESSION_HANDOVER.md +++ b/SESSION_HANDOVER.md @@ -1,75 +1,112 @@ -# Session Handoff: PR #59 Critical Fixes +# Session Handoff: [Issue #60] - Workflow Refactoring -**Date**: 2025-11-03 -**Issue**: #52 - Enhancement: Improve installation testing coverage -**Branch**: fix/issue-52-installation-testing -**PR**: (to be created) +**Date**: 2025-11-04 +**Issue**: #60 - Refactor GitHub Actions workflow to eliminate test duplication +**PR**: #64 - refactor: eliminate test duplication in workflow (resolves #60) +**Branch**: feat/issue-60-workflow-refactor --- -## ✅ Work Completed +## ✅ Completed Work -### Critical Fixes Applied (4 blockers) +### Workflow Refactoring +- **Eliminated duplication**: Removed ~240 lines of inline test logic from `.github/workflows/shell-quality.yml` +- **Single invocation**: Replaced with one call to `./tests/installation-test.sh` +- **Maintained functionality**: All 9 test scenarios still run, diagnostic artifacts still upload on failure +- **Result**: installation-test job reduced from ~240 lines to ~26 lines -1. **Security Fix (CVSS 9.0)**: Command injection vulnerability eliminated - - install.sh: Replaced `eval` with allowlist-based variable expansion - - Only safe patterns expanded: `${HOME}`, `${XDG_CONFIG_HOME}`, `$HOME`, `$XDG_CONFIG_HOME` - - Prevents arbitrary code execution via ZDOTDIR manipulation +### Changes Made +1. **File**: `.github/workflows/shell-quality.yml` + - Removed 9 separate inline test steps + - Added single step: `./tests/installation-test.sh "$TEMP_HOME" "$GITHUB_WORKSPACE"` + - Maintained diagnostic artifact upload on failure + - Kept temporary home directory setup -2. **Testing Fix**: Environment isolation for CI/local tests - - tests/installation-test.sh: Added `unset XDG_CONFIG_HOME XDG_DATA_HOME XDG_CACHE_HOME ZDOTDIR` - - .github/workflows/shell-quality.yml: Added environment isolation to all 3 install.sh invocations - - Prevents tests from inheriting parent environment settings +### Benefits Achieved +✅ Single source of truth for test logic +✅ Easier maintenance (update tests in one place) +✅ Guaranteed consistency between CI and local tests +✅ Reduced workflow complexity -3. **Code Quality**: Fixed 5 shellcheck SC2155 warnings - - tests/installation-test.sh lines 171, 252, 279, 303, 304 - - Separated local variable declaration from assignment - - Prevents masking command failures - -4. **Documentation**: Updated README.md with comprehensive test coverage - - Documented all 9 test scenarios from enhanced test suite - - Added standalone test script usage instructions - - Updated last modified date to 2025-11-03 +--- -5. **Formatting**: Applied shfmt formatting standards - - Fixed redirect spacing: `2> /dev/null` instead of `2>/dev/null` - - Fixed comment spacing: single space before inline comments +## 🎯 Current Project State + +**Tests**: ✅ All CI checks passing on PR #64 +**Branch**: feat/issue-60-workflow-refactor, clean working directory +**CI/CD**: ✅ All checks passed (9/9 passed, 0 failed) +**PR Status**: Draft, ready for review and merge + +### CI Check Results (PR #64) +- ✅ Scan for Secrets +- ✅ Shell Format Check +- ✅ ShellCheck +- ✅ **Test Installation Script** (refactored workflow - PASSES!) +- ✅ Detect AI Attribution Markers +- ✅ Check Conventional Commits +- ✅ Analyze Commit Quality +- ✅ Run Pre-commit Hooks +- ⏭️ Check Session Handoff Documentation (skipping - expected) + +### Agent Validation Status +- [x] architecture-designer: ✅ Complete (recommended this refactoring in issue #60) +- [ ] security-validator: N/A (no security changes) +- [x] code-quality-analyzer: ✅ Complete (YAML formatting verified) +- [ ] test-automation-qa: N/A (test logic unchanged, only location changed) +- [ ] performance-optimizer: N/A (no performance impact) +- [x] documentation-knowledge-manager: ✅ Complete (PR description documents changes) --- -## 🎯 Current State +## 🚀 Next Session Priorities -**Tests**: ✅ Docker build passes, install.sh creates all symlinks correctly -**Branch**: Up to date with origin, 2 commits ahead of master -**CI/CD**: Pending - awaiting final verification after latest fixes -**Security**: ✅ Command injection eliminated, no eval usage +**Immediate Next Steps:** +1. Review PR #64 and merge to master when ready +2. Close issue #60 (will auto-close via PR merge) +3. Consider tackling issue #61 (rollback script - HIGH priority) or #62 (shfmt caching - LOW priority) -### Commits in PR -1. `5d7ef4e` - feat: enhance installation testing coverage (resolves #52) -2. `d226206` - fix: resolve security, testing, and code quality issues +**Roadmap Context:** +- Issue #60: ✅ Complete (PR #64 ready to merge) +- Issue #61: Add automated rollback script (45-60 min, HIGH priority for production safety) +- Issue #62: Optimize CI with shfmt caching (10-15 min, LOW priority optimization) --- ## 📝 Startup Prompt for Next Session -Read CLAUDE.md to understand our workflow, then verify PR #59 CI passes and merge to master. +Read CLAUDE.md to understand our workflow, then review and merge PR #64 for issue #60. -**Immediate priority**: Verify CI pipeline passes (10 minutes) -**Context**: PR #59 fixes 4 critical blockers - security, testing, code quality, docs. All fixes applied and pushed. -**Reference docs**: PR #59, Issue #52, SESSION_HANDOVER.md (this file) -**Ready state**: Branch clean, all commits pushed, pre-commit hooks passing +**Immediate priority**: Review PR #64, merge to master (5-10 minutes) +**Context**: Issue #60 complete - workflow refactored successfully, all CI passing, eliminates ~240 lines of duplication +**Reference docs**: PR #64, Issue #60, SESSION_HANDOVER.md (this file) +**Ready state**: Clean master branch, PR #64 ready to merge, all tests passing -**Expected scope**: Monitor CI, address any remaining failures, merge PR #59 when green +**Expected scope**: Merge PR #64, then tackle issue #61 (rollback script) or #62 (shfmt caching) --- ## 📚 Key Reference Documents -- **PR #59**: https://github.com/maxrantil/dotfiles/pull/59 -- **Issue #52**: https://github.com/maxrantil/dotfiles/issues/52 -- **Previous session**: SESSION_HANDOVER.md from master branch (PR #55 context) +- **PR #64**: https://github.com/maxrantil/dotfiles/pull/64 +- **Issue #60**: https://github.com/maxrantil/dotfiles/issues/60 +- **Modified file**: `.github/workflows/shell-quality.yml` + +--- + +## 📋 Notes & Observations + +### Pre-existing Issue Identified +**Starship cache test failure**: Docker test (`./tests/docker-test.sh`) fails on Test 3 "Starship cache creation (Issue #7)" with message "Starship cache was not created". This failure: +- Also occurs on master branch (not caused by this PR) +- Is unrelated to the workflow refactoring +- May warrant a separate issue for investigation + +### Commit Details +- **Commit**: `3bcf746` - refactor: eliminate test duplication in workflow (resolves #60) +- **Files changed**: 1 file, 2 insertions(+), 223 deletions(-) +- **Pre-commit hooks**: All passed --- -**Session Status**: ✅ FIXES COMPLETE, AWAITING CI VERIFICATION -**Next Action**: Monitor CI pipeline, merge when green +**Session Status**: ✅ ISSUE #60 COMPLETE, PR #64 READY TO MERGE +**Next Action**: Review PR #64 and merge when ready, then select next issue (#61 or #62)