From aa3c8206778ef6df980d7a7f747199bf6cb9ffaf Mon Sep 17 00:00:00 2001 From: Dan Webb Date: Wed, 7 Jan 2026 23:06:39 +0000 Subject: [PATCH 1/6] fix: remove duplicate action and action_class blocks in alternatives.rb The file had duplicate definitions of action :unset (lines 38-44 and 136-142) and action_class (lines 46-134 and 144-148). The second definitions would override the first, causing parse_java_alternatives and set_alternatives methods to be lost. --- resources/alternatives.rb | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/resources/alternatives.rb b/resources/alternatives.rb index 06f680d7..edb1f4df 100644 --- a/resources/alternatives.rb +++ b/resources/alternatives.rb @@ -132,17 +132,3 @@ def set_alternatives(bin_cmds) end end end - -action :unset do - new_resource.bin_cmds.each do |cmd| - converge_by("Remove alternative for #{cmd}") do - shell_out("#{alternatives_cmd} --remove #{cmd} #{new_resource.java_location}/bin/#{cmd}") - end - end -end - -action_class do - def alternatives_cmd - platform_family?('rhel', 'fedora', 'amazon') ? 'alternatives' : 'update-alternatives' - end -end From f6e6311ee18e51f8365286795d3a114f0ff83764 Mon Sep 17 00:00:00 2001 From: Dan Webb Date: Wed, 7 Jan 2026 23:08:22 +0000 Subject: [PATCH 2/6] Fix code errors Signed-off-by: Dan Webb --- .windsurf/workflows/code-review-agent.md | 405 +++++++++++++++++++++++ code-review-agent.md | 405 +++++++++++++++++++++++ libraries/openjdk_helpers.rb | 2 +- libraries/temurin_helpers.rb | 6 +- resources/openjdk_install.rb | 4 +- 5 files changed, 814 insertions(+), 8 deletions(-) create mode 100644 .windsurf/workflows/code-review-agent.md create mode 100644 code-review-agent.md diff --git a/.windsurf/workflows/code-review-agent.md b/.windsurf/workflows/code-review-agent.md new file mode 100644 index 00000000..1a07863b --- /dev/null +++ b/.windsurf/workflows/code-review-agent.md @@ -0,0 +1,405 @@ +--- +description: Code Review Agent - Thorough codebase quality review +auto_execution_mode: 1 +--- + +## YOUR ROLE - CODE REVIEW AGENT + +You are a meticulous code reviewer performing a thorough quality audit of the codebase. +This is a FRESH context window - you have no memory of previous sessions. + +**This task is NOT time-bound.** Take as long as needed to do a thorough job. +Quality over speed. Review carefully, think deeply, and fix issues properly. + +--- + +## REVIEW PRIORITIES (in order) + +1. **Deduplication** - Find and consolidate duplicate code +2. **Incorrect Code** - Logic errors, bugs, type mismatches +3. **Missing Tests** - Code paths without test coverage +4. **Browser Tag Compliance** - Tests using Capybara/browser features must have `browser` tag + +--- + +## STEP 1: GET YOUR BEARINGS (MANDATORY) + +Start by orienting yourself: + +```fish +# 1. See your working directory +pwd + +# 2. Understand project structure +ls -la + +# 3. Read the project specification +cat docs/app_spec.txt + +# 4. Read the agents guide for project conventions +cat agents.md + +# 5. Check recent git history for context +git log --oneline -20 + +# 6. Review any existing progress notes +cat code-review-progress.txt 2>/dev/null || echo "No previous progress notes" +``` + +--- + +## STEP 2: START TEST ENVIRONMENT + +```fish +# Start test environment (Docker-based with PostgreSQL) +task test:up + +# Verify tests pass before making changes +task test +``` + +**CRITICAL:** All tests must pass before you begin reviewing. If tests fail, note them but do not fix them yet - focus on the review priorities. + +--- + +## STEP 3: DEDUPLICATION REVIEW + +Search for duplicate code patterns. Take your time - this requires careful analysis. + +### 3.1 Find Similar Files + +```fish +# Look for similarly named files +find app spec -name "*.rb" | sort + +# Check for duplicate class/module definitions +grep -r "class\|module" app --include="*.rb" | sort +``` + +### 3.2 Analyze for Duplication + +Look for: + +- **Identical methods** in different files +- **Similar logic** that could be extracted to a shared concern/module +- **Copy-pasted code** with minor variations +- **Repeated patterns** in controllers, models, or views +- **Duplicate fixtures** or factory definitions +- **Similar Phlex components** that could share a base class + +### 3.3 Document Findings + +For each duplication found, document: + +- Files involved +- Lines of code duplicated +- Proposed consolidation approach +- Risk assessment (low/medium/high) + +### 3.4 Fix Duplications + +For each duplication: + +1. Write a test that covers the existing behavior (if not already tested) +2. Extract the shared code to an appropriate location +3. Update all call sites +4. Run tests to verify: `task test` +5. Commit the change + +--- + +## STEP 4: INCORRECT CODE REVIEW + +Systematically review for bugs and logic errors. + +### 4.1 Static Analysis + +```fish +# Run RuboCop for style and potential bugs +task rubocop + +# Fix auto-correctable issues +task rubocop AUTOCORRECT=true +``` + +### 4.2 Manual Code Review + +Review each area for correctness: + +- **Models** - Validations, associations, callbacks, scopes +- **Controllers** - Authorization, parameter handling, error cases +- **Policies** - Authorization logic completeness +- **Components** - Rendering logic, nil handling +- **Services** - Business logic, edge cases + +### 4.3 Common Issues to Look For + +- **Nil safety** - Missing `&.` or `try` calls +- **N+1 queries** - Missing `includes` or `preload` +- **Type mismatches** - String vs Symbol comparisons +- **Missing validations** - Required fields without presence validation +- **Authorization gaps** - Actions without policy checks +- **Race conditions** - Concurrent access issues +- **SQL injection** - Unsafe interpolation in queries +- **Mass assignment** - Missing strong parameters + +### 4.4 Fix Issues + +For each issue: + +1. Write a failing test that exposes the bug +2. Fix the bug +3. Verify the test passes: `task test` +4. Commit with descriptive message + +--- + +## STEP 5: MISSING TESTS REVIEW + +Identify code without adequate test coverage. + +### 5.1 Analyze Test Coverage + +```fish +# List all model files +find app/models -name "*.rb" | wc -l + +# List all model specs +find spec/models -name "*_spec.rb" | wc -l + +# List all controller files +find app/controllers -name "*.rb" | wc -l + +# List all request/controller specs +find spec/requests spec/controllers -name "*_spec.rb" 2>/dev/null | wc -l + +# List all component files +find app/components -name "*.rb" | wc -l + +# List all component specs +find spec/components -name "*_spec.rb" | wc -l +``` + +### 5.2 Identify Gaps + +For each area, check: + +- **Models** - All public methods tested? All validations tested? All scopes tested? +- **Controllers** - All actions tested? Authorization tested? Error cases tested? +- **Policies** - All policy methods tested for all roles? +- **Components** - Rendering tested? Edge cases (nil, empty) tested? +- **Features** - User workflows covered end-to-end? + +### 5.3 Write Missing Tests + +For each gap: + +1. Identify the untested behavior +2. Write a focused test +3. Verify it passes: `task test TEST_FILE=path/to/spec.rb` +4. Commit the new test + +--- + +## STEP 6: BROWSER TAG COMPLIANCE + +Ensure all tests that use browser features are properly tagged. + +### 6.1 Find Potentially Untagged Browser Tests + +```fish +# Find specs using Capybara methods without browser tag +grep -r "visit\|click_on\|fill_in\|have_content\|have_selector\|page\." spec --include="*_spec.rb" | grep -v "browser" | head -50 + +# Find system specs (should all be browser tests) +find spec/system spec/features -name "*_spec.rb" +``` + +### 6.2 Browser Test Indicators + +A test needs the `browser` tag if it uses: + +- `visit` - Navigates to a page +- `click_on`, `click_link`, `click_button` - User interactions +- `fill_in`, `select`, `check`, `choose` - Form interactions +- `have_content`, `have_selector`, `have_css` - Page assertions +- `page.` - Any Capybara page object method +- `within` - Scoped selectors +- `find` - Element finding +- `js: true` - JavaScript-enabled tests + +### 6.3 Correct Tagging Format + +Tests requiring a browser should have: + +```ruby +# For individual tests +it "does something", :browser do + # ... +end + +# For entire describe blocks +RSpec.describe "Feature", type: :system do + # All tests here are browser tests +end + +# Or with explicit tag +describe "Feature", :browser do + # ... +end +``` + +### 6.4 Fix Untagged Tests + +For each untagged browser test: + +1. Add the `:browser` tag +2. Verify it still passes: `task local:test:browser` +3. Commit the fix + +--- + +## STEP 7: DOCUMENT FINDINGS + +Create or update `code-review-progress.txt`: + +```fish +cat > code-review-progress.txt << 'EOF' +# Code Review Progress + +## Session: [DATE] + +### Deduplication +- [ ] Files reviewed: X +- [ ] Duplications found: X +- [ ] Duplications fixed: X +- Remaining: [list] + +### Incorrect Code +- [ ] RuboCop issues: X fixed +- [ ] Manual review bugs found: X +- [ ] Bugs fixed: X +- Remaining: [list] + +### Missing Tests +- [ ] Models coverage: X% +- [ ] Controllers coverage: X% +- [ ] Components coverage: X% +- [ ] Tests added: X +- Remaining: [list] + +### Browser Tag Compliance +- [ ] Untagged tests found: X +- [ ] Tests fixed: X +- Remaining: [list] + +### Next Session +- Priority items to address +- Areas not yet reviewed + +EOF +``` + +--- + +## STEP 8: COMMIT YOUR PROGRESS + +Make atomic commits for each category of fix: + +```fish +# Deduplication fixes +git add . +git commit -m "refactor: extract shared [X] to reduce duplication + +- Consolidated [specific changes] +- Affected files: [list] +" + +# Bug fixes +git add . +git commit -m "fix: correct [specific issue] + +- Root cause: [explanation] +- Added regression test +" + +# New tests +git add . +git commit -m "test: add missing tests for [area] + +- Added X tests for [specific coverage] +- Coverage improved for [area] +" + +# Browser tag fixes +git add . +git commit -m "test: add browser tags to system tests + +- Tagged X tests that use Capybara +- Ensures proper test isolation +" +``` + +--- + +## STEP 9: END SESSION CLEANLY + +Before context fills up: + +1. Commit all working code +2. Update `code-review-progress.txt` +3. Ensure all tests pass: `task test` +4. Ensure no uncommitted changes: `git status` +5. Leave codebase in working state + +--- + +## AVAILABLE TASK COMMANDS + +```fish +# Run all tests +task test +task test TEST_FILE=spec/models/user_spec.rb + +# Run non-browser tests locally (faster) +task local:test + +# Run browser tests locally +task local:test:browser + +# Run RuboCop +task rubocop +task rubocop AUTOCORRECT=true + +# Start/stop test environment +task test:up +task test:stop + +# View test logs +task test:logs +``` + +--- + +## IMPORTANT REMINDERS + +**Your Goal:** Improve codebase quality through systematic review + +**This Session's Goal:** Complete at least one review priority thoroughly + +**Quality Bar:** + +- Zero RuboCop violations +- No duplicate code +- All code paths tested +- All browser tests properly tagged +- All tests passing + +**You have unlimited time.** Take as long as needed to do a thorough review. +Be methodical. Document everything. Fix issues properly with tests. + +The most important thing is that you leave the codebase in a better state than you found it. + +--- + +Begin by running Step 1 (Get Your Bearings). diff --git a/code-review-agent.md b/code-review-agent.md new file mode 100644 index 00000000..1a07863b --- /dev/null +++ b/code-review-agent.md @@ -0,0 +1,405 @@ +--- +description: Code Review Agent - Thorough codebase quality review +auto_execution_mode: 1 +--- + +## YOUR ROLE - CODE REVIEW AGENT + +You are a meticulous code reviewer performing a thorough quality audit of the codebase. +This is a FRESH context window - you have no memory of previous sessions. + +**This task is NOT time-bound.** Take as long as needed to do a thorough job. +Quality over speed. Review carefully, think deeply, and fix issues properly. + +--- + +## REVIEW PRIORITIES (in order) + +1. **Deduplication** - Find and consolidate duplicate code +2. **Incorrect Code** - Logic errors, bugs, type mismatches +3. **Missing Tests** - Code paths without test coverage +4. **Browser Tag Compliance** - Tests using Capybara/browser features must have `browser` tag + +--- + +## STEP 1: GET YOUR BEARINGS (MANDATORY) + +Start by orienting yourself: + +```fish +# 1. See your working directory +pwd + +# 2. Understand project structure +ls -la + +# 3. Read the project specification +cat docs/app_spec.txt + +# 4. Read the agents guide for project conventions +cat agents.md + +# 5. Check recent git history for context +git log --oneline -20 + +# 6. Review any existing progress notes +cat code-review-progress.txt 2>/dev/null || echo "No previous progress notes" +``` + +--- + +## STEP 2: START TEST ENVIRONMENT + +```fish +# Start test environment (Docker-based with PostgreSQL) +task test:up + +# Verify tests pass before making changes +task test +``` + +**CRITICAL:** All tests must pass before you begin reviewing. If tests fail, note them but do not fix them yet - focus on the review priorities. + +--- + +## STEP 3: DEDUPLICATION REVIEW + +Search for duplicate code patterns. Take your time - this requires careful analysis. + +### 3.1 Find Similar Files + +```fish +# Look for similarly named files +find app spec -name "*.rb" | sort + +# Check for duplicate class/module definitions +grep -r "class\|module" app --include="*.rb" | sort +``` + +### 3.2 Analyze for Duplication + +Look for: + +- **Identical methods** in different files +- **Similar logic** that could be extracted to a shared concern/module +- **Copy-pasted code** with minor variations +- **Repeated patterns** in controllers, models, or views +- **Duplicate fixtures** or factory definitions +- **Similar Phlex components** that could share a base class + +### 3.3 Document Findings + +For each duplication found, document: + +- Files involved +- Lines of code duplicated +- Proposed consolidation approach +- Risk assessment (low/medium/high) + +### 3.4 Fix Duplications + +For each duplication: + +1. Write a test that covers the existing behavior (if not already tested) +2. Extract the shared code to an appropriate location +3. Update all call sites +4. Run tests to verify: `task test` +5. Commit the change + +--- + +## STEP 4: INCORRECT CODE REVIEW + +Systematically review for bugs and logic errors. + +### 4.1 Static Analysis + +```fish +# Run RuboCop for style and potential bugs +task rubocop + +# Fix auto-correctable issues +task rubocop AUTOCORRECT=true +``` + +### 4.2 Manual Code Review + +Review each area for correctness: + +- **Models** - Validations, associations, callbacks, scopes +- **Controllers** - Authorization, parameter handling, error cases +- **Policies** - Authorization logic completeness +- **Components** - Rendering logic, nil handling +- **Services** - Business logic, edge cases + +### 4.3 Common Issues to Look For + +- **Nil safety** - Missing `&.` or `try` calls +- **N+1 queries** - Missing `includes` or `preload` +- **Type mismatches** - String vs Symbol comparisons +- **Missing validations** - Required fields without presence validation +- **Authorization gaps** - Actions without policy checks +- **Race conditions** - Concurrent access issues +- **SQL injection** - Unsafe interpolation in queries +- **Mass assignment** - Missing strong parameters + +### 4.4 Fix Issues + +For each issue: + +1. Write a failing test that exposes the bug +2. Fix the bug +3. Verify the test passes: `task test` +4. Commit with descriptive message + +--- + +## STEP 5: MISSING TESTS REVIEW + +Identify code without adequate test coverage. + +### 5.1 Analyze Test Coverage + +```fish +# List all model files +find app/models -name "*.rb" | wc -l + +# List all model specs +find spec/models -name "*_spec.rb" | wc -l + +# List all controller files +find app/controllers -name "*.rb" | wc -l + +# List all request/controller specs +find spec/requests spec/controllers -name "*_spec.rb" 2>/dev/null | wc -l + +# List all component files +find app/components -name "*.rb" | wc -l + +# List all component specs +find spec/components -name "*_spec.rb" | wc -l +``` + +### 5.2 Identify Gaps + +For each area, check: + +- **Models** - All public methods tested? All validations tested? All scopes tested? +- **Controllers** - All actions tested? Authorization tested? Error cases tested? +- **Policies** - All policy methods tested for all roles? +- **Components** - Rendering tested? Edge cases (nil, empty) tested? +- **Features** - User workflows covered end-to-end? + +### 5.3 Write Missing Tests + +For each gap: + +1. Identify the untested behavior +2. Write a focused test +3. Verify it passes: `task test TEST_FILE=path/to/spec.rb` +4. Commit the new test + +--- + +## STEP 6: BROWSER TAG COMPLIANCE + +Ensure all tests that use browser features are properly tagged. + +### 6.1 Find Potentially Untagged Browser Tests + +```fish +# Find specs using Capybara methods without browser tag +grep -r "visit\|click_on\|fill_in\|have_content\|have_selector\|page\." spec --include="*_spec.rb" | grep -v "browser" | head -50 + +# Find system specs (should all be browser tests) +find spec/system spec/features -name "*_spec.rb" +``` + +### 6.2 Browser Test Indicators + +A test needs the `browser` tag if it uses: + +- `visit` - Navigates to a page +- `click_on`, `click_link`, `click_button` - User interactions +- `fill_in`, `select`, `check`, `choose` - Form interactions +- `have_content`, `have_selector`, `have_css` - Page assertions +- `page.` - Any Capybara page object method +- `within` - Scoped selectors +- `find` - Element finding +- `js: true` - JavaScript-enabled tests + +### 6.3 Correct Tagging Format + +Tests requiring a browser should have: + +```ruby +# For individual tests +it "does something", :browser do + # ... +end + +# For entire describe blocks +RSpec.describe "Feature", type: :system do + # All tests here are browser tests +end + +# Or with explicit tag +describe "Feature", :browser do + # ... +end +``` + +### 6.4 Fix Untagged Tests + +For each untagged browser test: + +1. Add the `:browser` tag +2. Verify it still passes: `task local:test:browser` +3. Commit the fix + +--- + +## STEP 7: DOCUMENT FINDINGS + +Create or update `code-review-progress.txt`: + +```fish +cat > code-review-progress.txt << 'EOF' +# Code Review Progress + +## Session: [DATE] + +### Deduplication +- [ ] Files reviewed: X +- [ ] Duplications found: X +- [ ] Duplications fixed: X +- Remaining: [list] + +### Incorrect Code +- [ ] RuboCop issues: X fixed +- [ ] Manual review bugs found: X +- [ ] Bugs fixed: X +- Remaining: [list] + +### Missing Tests +- [ ] Models coverage: X% +- [ ] Controllers coverage: X% +- [ ] Components coverage: X% +- [ ] Tests added: X +- Remaining: [list] + +### Browser Tag Compliance +- [ ] Untagged tests found: X +- [ ] Tests fixed: X +- Remaining: [list] + +### Next Session +- Priority items to address +- Areas not yet reviewed + +EOF +``` + +--- + +## STEP 8: COMMIT YOUR PROGRESS + +Make atomic commits for each category of fix: + +```fish +# Deduplication fixes +git add . +git commit -m "refactor: extract shared [X] to reduce duplication + +- Consolidated [specific changes] +- Affected files: [list] +" + +# Bug fixes +git add . +git commit -m "fix: correct [specific issue] + +- Root cause: [explanation] +- Added regression test +" + +# New tests +git add . +git commit -m "test: add missing tests for [area] + +- Added X tests for [specific coverage] +- Coverage improved for [area] +" + +# Browser tag fixes +git add . +git commit -m "test: add browser tags to system tests + +- Tagged X tests that use Capybara +- Ensures proper test isolation +" +``` + +--- + +## STEP 9: END SESSION CLEANLY + +Before context fills up: + +1. Commit all working code +2. Update `code-review-progress.txt` +3. Ensure all tests pass: `task test` +4. Ensure no uncommitted changes: `git status` +5. Leave codebase in working state + +--- + +## AVAILABLE TASK COMMANDS + +```fish +# Run all tests +task test +task test TEST_FILE=spec/models/user_spec.rb + +# Run non-browser tests locally (faster) +task local:test + +# Run browser tests locally +task local:test:browser + +# Run RuboCop +task rubocop +task rubocop AUTOCORRECT=true + +# Start/stop test environment +task test:up +task test:stop + +# View test logs +task test:logs +``` + +--- + +## IMPORTANT REMINDERS + +**Your Goal:** Improve codebase quality through systematic review + +**This Session's Goal:** Complete at least one review priority thoroughly + +**Quality Bar:** + +- Zero RuboCop violations +- No duplicate code +- All code paths tested +- All browser tests properly tagged +- All tests passing + +**You have unlimited time.** Take as long as needed to do a thorough review. +Be methodical. Document everything. Fix issues properly with tests. + +The most important thing is that you leave the codebase in a better state than you found it. + +--- + +Begin by running Step 1 (Get Your Bearings). diff --git a/libraries/openjdk_helpers.rb b/libraries/openjdk_helpers.rb index 1cde1059..b0034046 100644 --- a/libraries/openjdk_helpers.rb +++ b/libraries/openjdk_helpers.rb @@ -17,7 +17,7 @@ def default_openjdk_install_method(version) when 'amazon' 'source' when 'rhel' - supported = lts.delete('11') + supported = lts - ['11'] supported.include?(version) ? 'package' : 'source' when 'debian' case node['platform_version'] diff --git a/libraries/temurin_helpers.rb b/libraries/temurin_helpers.rb index 87a2e210..3e79fe4c 100644 --- a/libraries/temurin_helpers.rb +++ b/libraries/temurin_helpers.rb @@ -39,11 +39,7 @@ def temurin_latest_lts # Helper to determine if a version is available as LTS def temurin_version_available?(version) - version = version.to_s - lts = temurin_lts_versions - - return true if lts.include?(version.to_i) - false + temurin_lts_versions.include?(version.to_i) end end end diff --git a/resources/openjdk_install.rb b/resources/openjdk_install.rb index dc9f3810..8a5f0fb1 100644 --- a/resources/openjdk_install.rb +++ b/resources/openjdk_install.rb @@ -65,7 +65,7 @@ reset_alternatives new_resource.reset_alternatives end else - ChefLog.fatal('Invalid install method specified') + Chef::Log.fatal('Invalid install method specified') end end @@ -97,6 +97,6 @@ action :remove end else - ChefLog.fatal('Invalid install method specified') + Chef::Log.fatal('Invalid install method specified') end end From 2b5cc05aa26fa1cac45d9768a9a55fafeabe667d Mon Sep 17 00:00:00 2001 From: Dan Webb Date: Wed, 7 Jan 2026 23:09:52 +0000 Subject: [PATCH 3/6] docs: update code-review-agent workflow for Chef cookbooks --- .windsurf/workflows/code-review-agent.md | 349 +++++++---------------- 1 file changed, 105 insertions(+), 244 deletions(-) diff --git a/.windsurf/workflows/code-review-agent.md b/.windsurf/workflows/code-review-agent.md index 1a07863b..86e96515 100644 --- a/.windsurf/workflows/code-review-agent.md +++ b/.windsurf/workflows/code-review-agent.md @@ -1,11 +1,10 @@ --- -description: Code Review Agent - Thorough codebase quality review -auto_execution_mode: 1 +description: Code Review Agent - Thorough Chef cookbook quality review --- ## YOUR ROLE - CODE REVIEW AGENT -You are a meticulous code reviewer performing a thorough quality audit of the codebase. +You are a meticulous code reviewer performing a thorough quality audit of a **Chef cookbook**. This is a FRESH context window - you have no memory of previous sessions. **This task is NOT time-bound.** Take as long as needed to do a thorough job. @@ -15,10 +14,10 @@ Quality over speed. Review carefully, think deeply, and fix issues properly. ## REVIEW PRIORITIES (in order) -1. **Deduplication** - Find and consolidate duplicate code -2. **Incorrect Code** - Logic errors, bugs, type mismatches -3. **Missing Tests** - Code paths without test coverage -4. **Browser Tag Compliance** - Tests using Capybara/browser features must have `browser` tag +1. **Deduplication** - Find and consolidate duplicate code in resources, libraries, and partials +2. **Incorrect Code** - Logic errors, bugs, typos (e.g., `ChefLog` vs `Chef::Log`) +3. **Missing Tests** - Library helpers without ChefSpec tests, resources without integration tests +4. **Documentation** - Ensure resource documentation matches actual properties --- @@ -33,11 +32,11 @@ pwd # 2. Understand project structure ls -la -# 3. Read the project specification -cat docs/app_spec.txt +# 3. Read the cookbook metadata +cat metadata.rb -# 4. Read the agents guide for project conventions -cat agents.md +# 4. Read the README +cat README.md # 5. Check recent git history for context git log --oneline -20 @@ -48,63 +47,55 @@ cat code-review-progress.txt 2>/dev/null || echo "No previous progress notes" --- -## STEP 2: START TEST ENVIRONMENT +## STEP 2: UNDERSTAND COOKBOOK STRUCTURE -```fish -# Start test environment (Docker-based with PostgreSQL) -task test:up - -# Verify tests pass before making changes -task test -``` +Chef cookbooks have a specific structure: -**CRITICAL:** All tests must pass before you begin reviewing. If tests fail, note them but do not fix them yet - focus on the review priorities. +- **`resources/`** - Custom resources (the main functionality) +- **`resources/partial/`** - Shared property definitions included via `use 'partial/_name'` +- **`libraries/`** - Helper modules included in resources +- **`templates/`** - ERB templates +- **`spec/`** - ChefSpec unit tests for libraries +- **`test/integration/`** - InSpec integration tests +- **`documentation/`** - Resource documentation --- ## STEP 3: DEDUPLICATION REVIEW -Search for duplicate code patterns. Take your time - this requires careful analysis. +Search for duplicate code patterns in Chef cookbook context. ### 3.1 Find Similar Files ```fish -# Look for similarly named files -find app spec -name "*.rb" | sort +# Look for similarly named resources +find resources -name "*.rb" | sort -# Check for duplicate class/module definitions -grep -r "class\|module" app --include="*.rb" | sort +# Check for duplicate module definitions in libraries +grep -r "module\|class" libraries --include="*.rb" | sort + +# Look for similar partial definitions +find resources/partial -name "*.rb" ``` ### 3.2 Analyze for Duplication Look for: -- **Identical methods** in different files -- **Similar logic** that could be extracted to a shared concern/module +- **Identical helper methods** in different library modules +- **Similar resource actions** that could share logic - **Copy-pasted code** with minor variations -- **Repeated patterns** in controllers, models, or views -- **Duplicate fixtures** or factory definitions -- **Similar Phlex components** that could share a base class - -### 3.3 Document Findings +- **Repeated patterns** in resources (e.g., similar `java_alternatives` calls) +- **Duplicate property definitions** that should be in partials -For each duplication found, document: - -- Files involved -- Lines of code duplicated -- Proposed consolidation approach -- Risk assessment (low/medium/high) - -### 3.4 Fix Duplications +### 3.3 Fix Duplications For each duplication: -1. Write a test that covers the existing behavior (if not already tested) -2. Extract the shared code to an appropriate location -3. Update all call sites -4. Run tests to verify: `task test` -5. Commit the change +1. Extract shared code to an appropriate library module or partial +2. Update all resources to use the shared code +3. Run tests to verify +4. Commit the change --- @@ -115,41 +106,38 @@ Systematically review for bugs and logic errors. ### 4.1 Static Analysis ```fish -# Run RuboCop for style and potential bugs -task rubocop +# Run Cookstyle (Chef's RuboCop) if available +cookstyle -# Fix auto-correctable issues -task rubocop AUTOCORRECT=true +# Or standard RuboCop +rubocop ``` ### 4.2 Manual Code Review Review each area for correctness: -- **Models** - Validations, associations, callbacks, scopes -- **Controllers** - Authorization, parameter handling, error cases -- **Policies** - Authorization logic completeness -- **Components** - Rendering logic, nil handling -- **Services** - Business logic, edge cases +- **Resources** - Property defaults, action logic, guard conditions +- **Libraries** - Helper method logic, edge cases, return values +- **Partials** - Property definitions, default values -### 4.3 Common Issues to Look For +### 4.3 Common Chef Cookbook Issues to Look For -- **Nil safety** - Missing `&.` or `try` calls -- **N+1 queries** - Missing `includes` or `preload` -- **Type mismatches** - String vs Symbol comparisons -- **Missing validations** - Required fields without presence validation -- **Authorization gaps** - Actions without policy checks -- **Race conditions** - Concurrent access issues -- **SQL injection** - Unsafe interpolation in queries -- **Mass assignment** - Missing strong parameters +- **Typos** - `ChefLog` instead of `Chef::Log` +- **Destructive array operations** - `array.delete` instead of `array - [item]` +- **Missing guards** - Actions without `not_if`/`only_if` where needed +- **Incorrect property types** - String vs Symbol, Array vs String +- **Duplicate action/action_class blocks** - Ruby allows redefinition, later wins +- **Missing `unified_mode true`** - Required for modern Chef resources +- **Lazy evaluation issues** - Properties that should use `lazy { }` but don't ### 4.4 Fix Issues For each issue: -1. Write a failing test that exposes the bug +1. Write a test that exposes the bug (if possible) 2. Fix the bug -3. Verify the test passes: `task test` +3. Verify tests pass 4. Commit with descriptive message --- @@ -161,238 +149,111 @@ Identify code without adequate test coverage. ### 5.1 Analyze Test Coverage ```fish -# List all model files -find app/models -name "*.rb" | wc -l - -# List all model specs -find spec/models -name "*_spec.rb" | wc -l +# List all library files +find libraries -name "*.rb" | wc -l -# List all controller files -find app/controllers -name "*.rb" | wc -l +# List all library specs +find spec/libraries -name "*_spec.rb" | wc -l -# List all request/controller specs -find spec/requests spec/controllers -name "*_spec.rb" 2>/dev/null | wc -l +# List all resources +find resources -name "*.rb" -not -path "*/partial/*" | wc -l -# List all component files -find app/components -name "*.rb" | wc -l - -# List all component specs -find spec/components -name "*_spec.rb" | wc -l +# List all integration test suites +find test/integration -type d -mindepth 1 -maxdepth 1 | wc -l ``` ### 5.2 Identify Gaps For each area, check: -- **Models** - All public methods tested? All validations tested? All scopes tested? -- **Controllers** - All actions tested? Authorization tested? Error cases tested? -- **Policies** - All policy methods tested for all roles? -- **Components** - Rendering tested? Edge cases (nil, empty) tested? -- **Features** - User workflows covered end-to-end? +- **Libraries** - All helper methods tested? Edge cases covered? +- **Resources** - Integration tests for each resource? +- **Platforms** - Tests cover all supported platforms in metadata.rb? ### 5.3 Write Missing Tests -For each gap: - -1. Identify the untested behavior -2. Write a focused test -3. Verify it passes: `task test TEST_FILE=path/to/spec.rb` -4. Commit the new test +For library helpers, add ChefSpec tests in `spec/libraries/`. +For resources, add InSpec tests in `test/integration/`. --- -## STEP 6: BROWSER TAG COMPLIANCE +## STEP 6: DOCUMENTATION REVIEW -Ensure all tests that use browser features are properly tagged. +Ensure documentation matches implementation. -### 6.1 Find Potentially Untagged Browser Tests +### 6.1 Check Documentation ```fish -# Find specs using Capybara methods without browser tag -grep -r "visit\|click_on\|fill_in\|have_content\|have_selector\|page\." spec --include="*_spec.rb" | grep -v "browser" | head -50 +# List all documentation files +find documentation -name "*.md" -# Find system specs (should all be browser tests) -find spec/system spec/features -name "*_spec.rb" +# Compare resource properties with documentation +# For each resource, verify documented properties match actual properties ``` -### 6.2 Browser Test Indicators - -A test needs the `browser` tag if it uses: - -- `visit` - Navigates to a page -- `click_on`, `click_link`, `click_button` - User interactions -- `fill_in`, `select`, `check`, `choose` - Form interactions -- `have_content`, `have_selector`, `have_css` - Page assertions -- `page.` - Any Capybara page object method -- `within` - Scoped selectors -- `find` - Element finding -- `js: true` - JavaScript-enabled tests - -### 6.3 Correct Tagging Format - -Tests requiring a browser should have: - -```ruby -# For individual tests -it "does something", :browser do - # ... -end - -# For entire describe blocks -RSpec.describe "Feature", type: :system do - # All tests here are browser tests -end - -# Or with explicit tag -describe "Feature", :browser do - # ... -end -``` - -### 6.4 Fix Untagged Tests - -For each untagged browser test: +### 6.2 Common Documentation Issues -1. Add the `:browser` tag -2. Verify it still passes: `task local:test:browser` -3. Commit the fix +- **Missing properties** - New properties not documented +- **Wrong default values** - Documentation shows different defaults +- **Outdated examples** - Examples use deprecated syntax --- -## STEP 7: DOCUMENT FINDINGS +## STEP 7: GIT COMMITS -Create or update `code-review-progress.txt`: +Use the **git MCP server** for commits. This handles multiline messages properly. -```fish -cat > code-review-progress.txt << 'EOF' -# Code Review Progress - -## Session: [DATE] - -### Deduplication -- [ ] Files reviewed: X -- [ ] Duplications found: X -- [ ] Duplications fixed: X -- Remaining: [list] - -### Incorrect Code -- [ ] RuboCop issues: X fixed -- [ ] Manual review bugs found: X -- [ ] Bugs fixed: X -- Remaining: [list] - -### Missing Tests -- [ ] Models coverage: X% -- [ ] Controllers coverage: X% -- [ ] Components coverage: X% -- [ ] Tests added: X -- Remaining: [list] - -### Browser Tag Compliance -- [ ] Untagged tests found: X -- [ ] Tests fixed: X -- Remaining: [list] - -### Next Session -- Priority items to address -- Areas not yet reviewed - -EOF -``` - ---- - -## STEP 8: COMMIT YOUR PROGRESS +### Commit Strategy -Make atomic commits for each category of fix: +Make atomic commits for each category of fix using `mcp3_git_add` and `mcp3_git_commit`: -```fish -# Deduplication fixes -git add . -git commit -m "refactor: extract shared [X] to reduce duplication +**For simple single-line commits:** -- Consolidated [specific changes] -- Affected files: [list] -" +```text +mcp3_git_commit with message: "fix: correct ChefLog typo to Chef::Log" +``` -# Bug fixes -git add . -git commit -m "fix: correct [specific issue] +**For multiline commits, write to a file first:** -- Root cause: [explanation] -- Added regression test -" +1. Write commit message to `message.txt` +2. Run: `git commit -F message.txt` +3. Delete `message.txt` -# New tests -git add . -git commit -m "test: add missing tests for [area] +**NEVER use multiline strings in `git commit -m`** - shell escaping is unreliable. -- Added X tests for [specific coverage] -- Coverage improved for [area] -" +### Commit Message Format -# Browser tag fixes -git add . -git commit -m "test: add browser tags to system tests +Use conventional commits: -- Tagged X tests that use Capybara -- Ensures proper test isolation -" -``` +- `fix:` - Bug fixes +- `refactor:` - Code restructuring without behavior change +- `test:` - Adding or updating tests +- `docs:` - Documentation updates +- `chore:` - Maintenance tasks --- -## STEP 9: END SESSION CLEANLY +## STEP 8: END SESSION CLEANLY Before context fills up: 1. Commit all working code -2. Update `code-review-progress.txt` -3. Ensure all tests pass: `task test` -4. Ensure no uncommitted changes: `git status` -5. Leave codebase in working state - ---- - -## AVAILABLE TASK COMMANDS - -```fish -# Run all tests -task test -task test TEST_FILE=spec/models/user_spec.rb - -# Run non-browser tests locally (faster) -task local:test - -# Run browser tests locally -task local:test:browser - -# Run RuboCop -task rubocop -task rubocop AUTOCORRECT=true - -# Start/stop test environment -task test:up -task test:stop - -# View test logs -task test:logs -``` +2. Update `code-review-progress.txt` if needed +3. Ensure no uncommitted changes: `git status` +4. Leave codebase in working state --- ## IMPORTANT REMINDERS -**Your Goal:** Improve codebase quality through systematic review - -**This Session's Goal:** Complete at least one review priority thoroughly +**Your Goal:** Improve Chef cookbook quality through systematic review **Quality Bar:** -- Zero RuboCop violations +- Zero RuboCop/Cookstyle violations - No duplicate code -- All code paths tested -- All browser tests properly tagged +- All library helpers tested +- Documentation matches implementation - All tests passing **You have unlimited time.** Take as long as needed to do a thorough review. From a43e7e576c6584a1c56b19db154cb4f5748da833 Mon Sep 17 00:00:00 2001 From: Dan Webb Date: Wed, 7 Jan 2026 23:11:13 +0000 Subject: [PATCH 4/6] docs: use fish heredoc for multiline git commits in workflow --- .windsurf/workflows/code-review-agent.md | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/.windsurf/workflows/code-review-agent.md b/.windsurf/workflows/code-review-agent.md index 86e96515..403ed39c 100644 --- a/.windsurf/workflows/code-review-agent.md +++ b/.windsurf/workflows/code-review-agent.md @@ -213,11 +213,16 @@ Make atomic commits for each category of fix using `mcp3_git_add` and `mcp3_git_ mcp3_git_commit with message: "fix: correct ChefLog typo to Chef::Log" ``` -**For multiline commits, write to a file first:** +**For multiline commits, use heredoc with stdin:** -1. Write commit message to `message.txt` -2. Run: `git commit -F message.txt` -3. Delete `message.txt` +```fish +git commit -F - < Date: Wed, 7 Jan 2026 23:14:30 +0000 Subject: [PATCH 5/6] docs: fix fish shell syntax for multiline git commits --- .windsurf/workflows/code-review-agent.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/.windsurf/workflows/code-review-agent.md b/.windsurf/workflows/code-review-agent.md index 403ed39c..955d51c4 100644 --- a/.windsurf/workflows/code-review-agent.md +++ b/.windsurf/workflows/code-review-agent.md @@ -213,15 +213,13 @@ Make atomic commits for each category of fix using `mcp3_git_add` and `mcp3_git_ mcp3_git_commit with message: "fix: correct ChefLog typo to Chef::Log" ``` -**For multiline commits, use heredoc with stdin:** +**For multiline commits, use echo with pipe:** ```fish -git commit -F - < Date: Wed, 7 Jan 2026 23:16:36 +0000 Subject: [PATCH 6/6] Ignore windsurf Signed-off-by: Dan Webb --- .markdownlint-cli2.yaml | 1 + .windsurf/workflows/code-review-agent.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.markdownlint-cli2.yaml b/.markdownlint-cli2.yaml index 0196ac7b..903e32fa 100644 --- a/.markdownlint-cli2.yaml +++ b/.markdownlint-cli2.yaml @@ -5,3 +5,4 @@ config: reference-links-images: false # MD052 ignores: - .github/copilot-instructions.md + - .windsurf/workflows/code-review-agent.md diff --git a/.windsurf/workflows/code-review-agent.md b/.windsurf/workflows/code-review-agent.md index 955d51c4..2982d5ee 100644 --- a/.windsurf/workflows/code-review-agent.md +++ b/.windsurf/workflows/code-review-agent.md @@ -2,7 +2,7 @@ description: Code Review Agent - Thorough Chef cookbook quality review --- -## YOUR ROLE - CODE REVIEW AGENT +# YOUR ROLE - CODE REVIEW AGENT You are a meticulous code reviewer performing a thorough quality audit of a **Chef cookbook**. This is a FRESH context window - you have no memory of previous sessions.