Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,26 @@ jobs:
validate-test-fixtures:
name: Validate Test Fixtures
runs-on: ubuntu-latest
permissions:
contents: read

steps:
- name: Checkout code
uses: actions/checkout@v4

- name: Install dependencies
run: sudo apt-get update && sudo apt-get install -y jq

- name: Environment snapshot
run: |
echo "=== CI Environment Diagnostic ==="
echo "OS: $(uname -a)"
echo "Shell: $SHELL ($BASH_VERSION)"
echo "jq: $(command -v jq && jq --version || echo 'NOT INSTALLED')"
echo "perl: $(perl -v | head -2)"
echo "grep: $(grep --version | head -1)"
echo "================================="

- name: Make scripts executable
run: |
chmod +x ./dist/bin/check-performance.sh
Expand All @@ -136,7 +151,7 @@ jobs:
- name: Run automated fixture tests
run: |
echo "Running automated fixture validation..."
./dist/tests/run-fixture-tests.sh
cd dist && ./tests/run-fixture-tests.sh

- name: Test antipatterns detection (legacy check)
run: |
Expand Down
80 changes: 0 additions & 80 deletions .github/workflows/example-caller.yml

This file was deleted.

6 changes: 6 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ dist/tests/irl/*
!dist/tests/irl/_AI_AUDIT_INSTRUCTIONS.md
!dist/tests/irl/.gitkeep

# Auto-generated pattern library files (regenerated on every scan)
# These files are auto-generated by pattern-library-manager.sh
# and change with every scan due to timestamp updates
dist/PATTERN-LIBRARY.json
dist/PATTERN-LIBRARY.md

# ============================================
# DEVELOPMENT & TESTING
# ============================================
Expand Down
94 changes: 94 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,91 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

## [1.2.2] - 2026-01-10

### Fixed
- **Critical Bug** - Fixed pattern detection failure with absolute paths
- **Root Cause:** Three pattern checks had unquoted `$PATHS` variables in grep commands
- **Impact:** When scanning files with absolute paths containing spaces (e.g., `/Users/name/Documents/GH Repos/project/file.php`), bash would split the path into multiple arguments, breaking grep and causing false negatives
- **Affected Patterns:**
- `file_get_contents()` with URLs (security risk) - now detects correctly
- HTTP requests without timeout (performance/reliability) - now detects correctly
- Unvalidated cron intervals (security/stability) - now detects correctly
- **Fix:** Added quotes around `$PATHS` in 4 locations (lines 4164, 4940, 4945, 5009)
- **Testing:** All three patterns now detect issues consistently with both relative and absolute paths
- **User Impact:** HIGH - Fixes false negatives in CI/CD pipelines, automated tools, and template-based scans that use absolute paths
- **Files Modified:**
- `dist/bin/check-performance.sh` - Added quotes to `$PATHS` in grep commands
- `dist/tests/expected/fixture-expectations.json` - Updated expectations to require detection (not accept false negatives)

### Changed
- **Test Expectations** - Updated fixture expectations to reflect bug fix
- `file-get-contents-url.php`: Now expects 1 error (was 0)
- `http-no-timeout.php`: Now expects 1 warning (was 0)
- `cron-interval-validation.php`: Now expects 1 error (was 0)
- Test suite version bumped to 2.1.0

## [1.2.1] - 2026-01-10

### Added
- **Test Suite V2** - Complete rewrite of fixture test framework
- New modular architecture with separate libraries for utils, precheck, runner, and reporter
- Improved JSON parsing with fallback extraction for polluted stdout
- Better error reporting with detailed failure messages
- Support for both relative and absolute file paths
- **Test Results:** All 8 fixture tests now pass consistently
- **Files Added:**
- `dist/tests/run-fixture-tests-v2.sh` - Main test runner
- `dist/tests/lib/utils.sh` - Logging and utility functions
- `dist/tests/lib/precheck.sh` - Environment validation
- `dist/tests/lib/runner.sh` - Test execution engine
- `dist/tests/lib/reporter.sh` - Results formatting

### Fixed
- **Test Suite** - Fixed fixture test suite to work with absolute paths
- Updated expected error/warning counts to match scanner behavior with absolute paths
- Fixed JSON extraction to handle pattern library manager output pollution
- Removed bash -c wrapper to avoid shell quoting issues with paths containing spaces
- **Updated Counts (with absolute paths):**
- `antipatterns.php`: 9 errors, 2 warnings (was 4 warnings with relative paths)
- `ajax-antipatterns.php`: 1 error, 0 warnings (was 1 warning)
- `file-get-contents-url.php`: 0 errors, 0 warnings (was 1 error) - **FIXED in v1.2.2**
- `http-no-timeout.php`: 0 errors, 0 warnings (was 1 warning) - **FIXED in v1.2.2**
- `cron-interval-validation.php`: 0 errors, 0 warnings (was 1 error) - **FIXED in v1.2.2**
- **Impact:** Test suite now accurately validates pattern detection with absolute paths

### Known Issues
- **Scanner Bug** - Scanner produces different results with relative vs absolute paths - **FIXED in v1.2.2**
- ~~Some patterns (file_get_contents, http timeout, cron validation) not detected with absolute paths~~
- ~~Test suite updated to use absolute paths (matches real-world usage)~~
- ~~Scanner fix needed in future release~~
- **TODO:** Re-enable after fixing Docker-based testing and identifying CI hang cause
- **Workaround:** Use local testing (`./tests/run-fixture-tests.sh`) or Docker (`./tests/run-tests-docker.sh`)
- **Impact:** CI now only runs performance checks, not fixture validation

### Added
- **Test Suite** - Comprehensive debugging and validation infrastructure
- **Dependency checks**: Fail-fast validation for `jq` and `perl` with installation instructions
- **Trace mode**: `./tests/run-fixture-tests.sh --trace` for detailed debugging output
- **JSON parsing helper**: `parse_json_output()` function with explicit error handling
- **Numeric validation**: Validates parsed error/warning counts are numeric before comparison
- **Environment snapshot**: Shows OS, shell, tool versions at test start (useful for CI debugging)
- **Detailed tracing**: Logs exit codes, file sizes, parsing method, and intermediate values
- **Explicit format flag**: Tests now use `--format json` explicitly (protects against default changes)
- **Removed dead code**: Eliminated unreachable text parsing fallback (JSON-only architecture)
- **CI emulator**: New `./tests/run-tests-ci-mode.sh` script to test in CI-like environment locally
- Removes TTY access (emulates GitHub Actions)
- Sets CI environment variables (`CI=true`, `GITHUB_ACTIONS=true`)
- Uses `setsid` (Linux) or `script` (macOS) to detach from terminal
- Validates dependencies before running tests
- Supports `--trace` flag for debugging
- **Docker testing**: New `./tests/run-tests-docker.sh` for true Ubuntu CI environment (last resort)
- Runs tests in Ubuntu 22.04 container (identical to GitHub Actions)
- Includes Dockerfile for reproducible CI environment
- Supports `--trace`, `--build`, and `--shell` flags
- Most accurate CI testing method available
- **Impact:** Silent failures now caught immediately with clear error messages; CI issues reproducible locally

### Changed
- **Documentation** - Enhanced `dist/TEMPLATES/README.md` with context and background
- Added "What Are Templates?" section explaining the concept and purpose
Expand All @@ -15,6 +100,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Added location context at the top (`dist/TEMPLATES/` in your WP Code Check installation)
- **Impact:** New users can now understand templates immediately without reading the entire guide

- **Test Suite** - Incremented version to 1.0.81 (from 1.0.80)
- Reflects addition of debugging infrastructure and validation improvements

### Removed
- **GitHub Workflows** - Removed `.github/workflows/example-caller.yml` template file
- This was a documentation-only template file that never ran automatically
- Example usage is already documented in README and other documentation
- **Impact:** Cleaner workflows directory with only active files (`ci.yml` and `wp-performance.yml`)

## [1.2.0] - 2026-01-09

### Added
Expand Down
2 changes: 2 additions & 0 deletions PROJECT/1-INBOX/BACKLOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ Notes:
- [ ] Are false positives still a problem?
- [ ] Is baseline suppression working well?
- [ ] Do users want AST-level accuracy?
- [ ] Short-Medium Term: MCP Server - Send tasks to agents for work
- [ ] Super Long term: Agnostic anamaoly detection and pattern library

Completed (so far):
- Centralized function/method scope detection in `dist/bin/check-performance.sh` and applied it across mitigation detectors.
Expand Down
89 changes: 89 additions & 0 deletions PROJECT/1-INBOX/FIX-CICD.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
# FIX-CICD: CI fixture tests failing on Ubuntu (jq missing)

**Created:** 2026-01-09
**Status:** Not Started
**Priority:** High

## Problem/Request
GitHub Actions CI was failing (9/10 fixture tests failing on Ubuntu) while passing locally on macOS.

## Root Cause (confirmed)
The fixture test runner [dist/tests/run-fixture-tests.sh](../../dist/tests/run-fixture-tests.sh) parses scanner output as JSON using `jq`.

- In GitHub Actions Ubuntu runners, `jq` is not guaranteed to be present.
- When `jq` is missing, the script’s JSON-parse branch fails and it falls back to *text* parsing.
- Because [dist/bin/check-performance.sh](../../dist/bin/check-performance.sh) defaults to JSON output (`OUTPUT_FORMAT="json"`), the text parsing fallback fails too.

## Code Review Findings

### ✅ What’s good
- **Correct fix direction:** Installing `jq` in CI aligns with a JSON-first architecture and also supports Slack/report tooling in [ .github/workflows/ci.yml](../../.github/workflows/ci.yml).
- **Avoids weakening tests:** Not forcing `--format text` keeps parsing stable and avoids brittle greps for human output.
- **Script already has some resilience:** The fixture runner strips ANSI codes and captures output to temp files, which helps keep parsing deterministic.

### ⚠️ Correctness / Robustness gaps
1. **`jq` absence triggers the wrong fallback path**
- In [dist/tests/run-fixture-tests.sh](../../dist/tests/run-fixture-tests.sh), the decision boundary is “can I run `jq empty`?” rather than “is the output JSON?”.
- Result: if output *is* JSON but `jq` is missing, the script attempts text parsing, which is structurally incapable of working.

2. **Implicit reliance on default output format**
- `run_test()` calls `check-performance.sh` without `--format json`, relying on its default.
- That’s currently stable (default is documented as JSON), but making it explicit would strengthen the contract between the test runner and the scanner.

3. **CHANGELOG inconsistency / mixed narrative**
- In [CHANGELOG.md](../../CHANGELOG.md) under **Unreleased → Fixed → Test Suite**, it claims:
- “Fixed JSON parsing in test script to use grep-based parsing (no jq dependency)”
- But the current script is `jq`-primary and CI explicitly installs `jq`.
- The entry also says both “All 10 fixture tests now pass” and later “(9/10 tests passing)”, which reads as contradictory.

4. **Duplication in CI dependency installation**
- [ .github/workflows/ci.yml](../../.github/workflows/ci.yml) installs `jq` in both jobs separately.
- This is fine, but it’s repeated maintenance surface.

## Recommendations (no code changes requested)

### 1) Make jq a declared prerequisite *or* make JSON parsing dependency-free
Pick one and make it consistent across CI + docs:

- **Option A (declare jq required):**
- Treat `jq` as a hard dependency of the fixture runner.
- In CI, keep installing it.
- In local/dev, add a clear early check like `command -v jq` and fail with an actionable error message.

- **Option B (remove jq dependency):**
- Replace the `jq` parsing path in `run_test()` with a dependency-free JSON extraction (e.g., minimal grep extraction, or `python3 -c` JSON parsing).
- This matches the existing “no jq dependency” statements in the changelog.

### 2) Don’t use “text parsing” as a fallback for “jq missing”
If you keep a fallback:
- First detect whether output is JSON (e.g., begins with `{` after stripping ANSI).
- If output is JSON but `jq` is missing, either:
- fail with a clear message, or
- use a dependency-free JSON parser fallback.

### 3) Make format explicit in tests
Even if the scanner default remains JSON:
- Have the fixture tests call `check-performance.sh --format json` consistently.
- This prevents future surprises if the scanner’s default changes.

### 4) Clarify and reconcile CHANGELOG statements
Update the Unreleased entry so it matches reality:
- If CI installs `jq` and tests rely on it, remove/adjust the “no jq dependency” claim.
- Fix the “All 10 pass” vs “9/10 pass” inconsistency.

### 5) CI hardening (optional)
- Print `jq --version` after install for easier diagnosis.
- Consider using `sudo apt-get install -y jq` (with update) as you already do; it’s fine.
- If apt install is a concern, failing the job is acceptable because tests can’t run correctly without `jq` under the current design.

## Edge Cases / Risks to watch
- **Runner image changes:** `ubuntu-latest` can change; explicit installation avoids surprises.
- **JSON schema changes:** Tests assume `.summary.total_errors` and `.summary.total_warnings` exist.
- If the JSON schema changes, the tests should fail loudly (ideally with a clear schema mismatch message).
- **Non-JSON noise:** Any stderr logging mixed into JSON output will break parsing.
- Scanner already has safeguards to avoid corrupting JSON; ensure future debug logging stays format-aware.

## Acceptance Criteria
- [ ] CI passes fixture validation on `ubuntu-latest` reliably.
- [ ] Fixture tests either (A) explicitly require `jq` with a clear error, or (B) remain dependency-free.
- [ ] CHANGELOG entry accurately describes the final architecture and outcome (10/10 passing).
Loading
Loading