From 38005a241d42aa038927394b89feda85a70c4cb5 Mon Sep 17 00:00:00 2001 From: Akkari Date: Mon, 16 Mar 2026 02:29:22 -0700 Subject: [PATCH] fix: pre-existing code quality findings + patch __version__ to 0.7.3 Co-Authored-By: Claude Opus 4.6 --- SPEC.md | 31 ++++++---- critic-status.yaml | 2 +- docs/CHANGELOG.md | 14 +++++ docs/architecture/IMPLEMENTATION.md | 2 +- docs/critics/CODE_HYGIENE_FRAMEWORK.md | 2 +- docs/critics/SECURITY_CRITIC_FRAMEWORK.md | 2 +- reference-implementation/pyproject.toml | 2 +- reference-implementation/quorum/__init__.py | 2 +- reference-implementation/quorum/prescreen.py | 19 +++--- .../tests/test_batch_resilience.py | 4 +- reference-implementation/tests/test_cli.py | 2 +- tools/validate-docs.py | 59 ++++++++----------- 12 files changed, 80 insertions(+), 61 deletions(-) diff --git a/SPEC.md b/SPEC.md index e3ed440..cdf60dc 100644 --- a/SPEC.md +++ b/SPEC.md @@ -30,17 +30,28 @@ A single model reviewing a long prompt generates: - Hand-waving without evidence (LLMs can justify anything) - ~~No learning across validations~~ → **Solved in v0.5.3:** Learning memory tracks recurring patterns and promotes high-frequency findings to mandatory checks -Quorum uses **nine specialized critics in parallel**, each with deep expertise in: +Quorum's target architecture has **nine specialized agents** — six critics are currently shipped, with three more planned: + +**Shipped critics (6):** 1. **Correctness Critic** — Factual accuracy, logical consistency, claim support -2. **Security Critic** — Vulnerability patterns, permission issues, injection risks -3. **Completeness Critic** — Coverage gaps, missing requirements, unaddressed edge cases -4. **Architecture Critic** — Design coherence, pattern consistency, scalability concerns -5. **Delegation & Coordination Critic** — Span of control, reversibility, bidirectional contracts -6. **Tester Agent** — Executes validation (schema checks, git queries, web searches, shell execution) -7. **Fixer Agent** — Generates fixes for CRITICAL/HIGH issues (optional, 1-2 loops max) -8. **Aggregator Critic** — Merges findings, resolves conflicts, recalibrates confidence -9. **Supervisor Agent** — Manages workflow, checkpoints, final verdict +2. **Completeness Critic** — Coverage gaps, missing requirements, unaddressed edge cases +3. **Security Critic** — Vulnerability patterns, permission issues, injection risks +4. **Code Hygiene Critic** — ISO 25010 maintainability, naming, complexity metrics +5. **Cross-Artifact Consistency Critic**† — Inter-file consistency (activated with `--relationships`) +6. **Tester Agent** — Finding verification: deterministic checks + LLM claim validation + +**Shipped infrastructure agents:** + +7. **Fixer Agent** — Proposes and applies fixes for CRITICAL/HIGH findings (optional, 1-2 loops max) +8. **Aggregator** — Merges findings, resolves conflicts, recalibrates confidence +9. **Supervisor** — Manages workflow, checkpoints, final verdict + +**Planned critics (not yet built):** + +10. **Architecture Critic** — Design coherence, pattern consistency, scalability concerns +11. **Delegation & Coordination Critic** — Span of control, reversibility, bidirectional contracts +12. **Style Critic** — Writing quality, tone consistency, formatting standards Critics don't vote. The Aggregator synthesizes their findings into a final verdict with explicit confidence levels. @@ -398,7 +409,7 @@ Additional runs on related artifacts reuse critic prompts and tools, amortizing ## 8. Implementation Checklist -Status as of v0.7.2 (reference implementation): +Status as of v0.7.3 (reference implementation): - [x] LLM provider — LiteLLM universal provider (100+ models, any tier combination) - [x] File-based artifact passing (no in-memory state between agents) diff --git a/critic-status.yaml b/critic-status.yaml index c7abcac..f2dc983 100644 --- a/critic-status.yaml +++ b/critic-status.yaml @@ -2,7 +2,7 @@ # Used by tools/validate-docs.py to detect stale documentation. # Update this file FIRST when shipping a critic or feature. -version: "0.7.2" +version: "0.7.3" spec_version: "3.0" critics: diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index fc5c15b..dab1b63 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -4,6 +4,20 @@ All notable changes to Quorum will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). +## [0.7.3] — 2026-03-15 + +### Fixed — Pre-Existing Code Findings Cleanup + +- **`__version__` corrected** — `reference-implementation/quorum/__init__.py` now reads `0.7.3` (was stuck at `0.5.3` since initial release; PyPI showed `0.7.2` but runtime `__version__` did not match) +- **Broad exception handling narrowed** (prescreen.py) — PS-006 `except Exception` narrowed to `(OSError, UnicodeDecodeError)`; external tool integrations (Ruff, DevSkim, Bandit, PSSA) now log with `exc_info=True` for traceback capture on unexpected failures +- **PSScriptAnalyzer security documentation** (prescreen.py) — Added comprehensive `# SECURITY:` comment explaining the PowerShell single-quote injection trust model for `_run_pssa` +- **SPEC.md §2.1 overclaiming fixed** — Numbered agent list now separates shipped critics (6) from planned critics (3) and infrastructure agents; Code Hygiene and Cross-Artifact Consistency added (were missing from list despite being shipped) +- **validate-docs.py hardcoded range removed** — `check_hardcoded_counts` `range(2, 9)` replaced with parameterized bounds derived from manifest total critic count +- **validate-docs.py SRP cleanup** — Extracted `_extract_pyproject_version()` and `_extract_badge_versions()` as pure functions; `main()` refactored to delegate to `validate_docs()` (eliminated duplicated validation loop) +- **Test exception handling** (test_batch_resilience.py) — Broad `except Exception` narrowed to `(json.JSONDecodeError, OSError)` with explanatory comment + +--- + ## [0.7.2] — 2026-03-12 ### Documentation — Stale Content Cleanup (PR #18) diff --git a/docs/architecture/IMPLEMENTATION.md b/docs/architecture/IMPLEMENTATION.md index b5d29f2..067cdff 100644 --- a/docs/architecture/IMPLEMENTATION.md +++ b/docs/architecture/IMPLEMENTATION.md @@ -2,7 +2,7 @@ This guide walks through building Quorum from the architectural spec. It's structured as a reference walkthrough, not a copy-paste tutorial. The goal is understanding the patterns so you can adapt them to your stack. -> **Implementation Status (v0.7.2):** This guide describes the full target architecture. All 6 critics are shipped and callable: Correctness, Completeness, Security, Code Hygiene, Cross-Artifact Consistency, and Tester. Also shipped: the fixer (proposal mode), parallel execution, batch processing, and pre-screen integration. See `critic-status.yaml` for the authoritative status matrix. Sections below marked 🔜 describe components not yet built (Architecture, Delegation, Style). +> **Implementation Status (v0.7.3):** This guide describes the full target architecture. All 6 critics are shipped and callable: Correctness, Completeness, Security, Code Hygiene, Cross-Artifact Consistency, and Tester. Also shipped: the fixer (proposal mode), parallel execution, batch processing, and pre-screen integration. See `critic-status.yaml` for the authoritative status matrix. Sections below marked 🔜 describe components not yet built (Architecture, Delegation, Style). --- diff --git a/docs/critics/CODE_HYGIENE_FRAMEWORK.md b/docs/critics/CODE_HYGIENE_FRAMEWORK.md index 8ac7c09..a9ce4ba 100644 --- a/docs/critics/CODE_HYGIENE_FRAMEWORK.md +++ b/docs/critics/CODE_HYGIENE_FRAMEWORK.md @@ -21,7 +21,7 @@ The boundary is clean: when code hygiene detects a pattern that has security imp ## Status -**v0.7.2 State:** Framework design complete. All features shipped. +**v0.7.3 State:** Framework design complete. All features shipped. - [x] Framework design and documentation - [x] 12 evaluation categories specified diff --git a/docs/critics/SECURITY_CRITIC_FRAMEWORK.md b/docs/critics/SECURITY_CRITIC_FRAMEWORK.md index 9eec96a..4c788cf 100644 --- a/docs/critics/SECURITY_CRITIC_FRAMEWORK.md +++ b/docs/critics/SECURITY_CRITIC_FRAMEWORK.md @@ -11,7 +11,7 @@ ## Status -**v0.7.2 State:** Framework design complete. All features shipped. +**v0.7.3 State:** Framework design complete. All features shipped. - [x] Framework design and documentation - [x] 14 evaluation categories (SEC-01–SEC-14) specified diff --git a/reference-implementation/pyproject.toml b/reference-implementation/pyproject.toml index 93011c0..adc4ea8 100644 --- a/reference-implementation/pyproject.toml +++ b/reference-implementation/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "quorum-validator" -version = "0.7.2" +version = "0.7.3" description = "Multi-critic quality validation for agents, research, and configurations" authors = [ {name = "Daniel Cervera", email = "daniel@sharedintellect.com"}, diff --git a/reference-implementation/quorum/__init__.py b/reference-implementation/quorum/__init__.py index 2c0c485..d827b1a 100644 --- a/reference-implementation/quorum/__init__.py +++ b/reference-implementation/quorum/__init__.py @@ -9,5 +9,5 @@ specialized critics, each required to provide grounded evidence. """ -__version__ = "0.5.3" +__version__ = "0.7.3" __all__ = ["__version__"] diff --git a/reference-implementation/quorum/prescreen.py b/reference-implementation/quorum/prescreen.py index dc7a699..b662dc2 100644 --- a/reference-implementation/quorum/prescreen.py +++ b/reference-implementation/quorum/prescreen.py @@ -240,8 +240,7 @@ def run(self, artifact_path: Path, artifact_text: str) -> PreScreenResult: ruff_checks = self._findings_to_checks(ruff_findings, "ruff") checks.extend(ruff_checks) except Exception as e: - logger.warning("Ruff integration failed: %s", e) - # Graceful degradation: treat as "no issues found" when tool fails + logger.warning("Ruff integration failed: %s", e, exc_info=True) checks.append(_pass("EXT-RUFF", "ruff_analysis", "external_tools", Severity.INFO, "No issues found by ruff (tool unavailable)")) @@ -251,8 +250,7 @@ def run(self, artifact_path: Path, artifact_text: str) -> PreScreenResult: devskim_checks = self._findings_to_checks(devskim_findings, "devskim") checks.extend(devskim_checks) except Exception as e: - logger.warning("DevSkim integration failed: %s", e) - # Graceful degradation: treat as "no issues found" when tool fails + logger.warning("DevSkim integration failed: %s", e, exc_info=True) checks.append(_pass("EXT-DEVSKIM", "devskim_analysis", "external_tools", Severity.INFO, "No issues found by devskim (tool unavailable)")) @@ -264,7 +262,7 @@ def run(self, artifact_path: Path, artifact_text: str) -> PreScreenResult: bandit_checks = self._findings_to_checks(bandit_findings, "bandit") checks.extend(bandit_checks) except Exception as e: - logger.warning("Bandit integration failed: %s", e) + logger.warning("Bandit integration failed: %s", e, exc_info=True) checks.append(_pass("EXT-BANDIT", "bandit_analysis", "external_tools", Severity.INFO, "No issues found by bandit (tool unavailable)")) @@ -275,7 +273,7 @@ def run(self, artifact_path: Path, artifact_text: str) -> PreScreenResult: pssa_checks = self._findings_to_checks(pssa_findings, "pssa") checks.extend(pssa_checks) except Exception as e: - logger.warning("PSScriptAnalyzer integration failed: %s", e) + logger.warning("PSScriptAnalyzer integration failed: %s", e, exc_info=True) checks.append(_pass("EXT-PSSA", "pssa_analysis", "external_tools", Severity.INFO, "No issues found by pssa (tool unavailable)")) @@ -447,9 +445,9 @@ def _ps006_python_syntax( f"Python syntax error at {loc}", msg, [loc] if loc_match else []) - except Exception as exc: + except (OSError, UnicodeDecodeError) as exc: return _skip("PS-006", "python_syntax", "syntax", Severity.HIGH, - "Python syntax check", f"Could not compile: {exc}") + "Python syntax check", f"Could not compile ({type(exc).__name__}): {exc}") finally: if tmp_path: @@ -855,7 +853,10 @@ def _run_pssa(self, artifact_path: Path, artifact_text: str) -> list[Finding]: } try: - # Escape single quotes in path to prevent PowerShell command injection + # SECURITY: PowerShell command injection mitigated. Single-quoted strings + # in PowerShell don't expand variables/backticks/subexpressions; only + # internal single quotes need doubling (' → ''), which we do below. + # artifact_path originates from filesystem resolution, not user input. escaped_path = str(artifact_path).replace("'", "''") ps_command = ( f"Invoke-ScriptAnalyzer -Path '{escaped_path}' " diff --git a/reference-implementation/tests/test_batch_resilience.py b/reference-implementation/tests/test_batch_resilience.py index cd5b821..2308df4 100644 --- a/reference-implementation/tests/test_batch_resilience.py +++ b/reference-implementation/tests/test_batch_resilience.py @@ -138,8 +138,8 @@ def spy_validate(*args, **kwargs): for p in (tmp_path / "runs").glob("batch-*/batch-manifest.json"): try: manifests_seen.append(json.loads(p.read_text())) - except Exception: - pass + except (json.JSONDecodeError, OSError): + pass # manifest may be partially written during race return original_validate(*args, **kwargs) # Create 3 files diff --git a/reference-implementation/tests/test_cli.py b/reference-implementation/tests/test_cli.py index bd3b1fd..24994bd 100644 --- a/reference-implementation/tests/test_cli.py +++ b/reference-implementation/tests/test_cli.py @@ -29,7 +29,7 @@ def test_help(self, runner): def test_version(self, runner): result = runner.invoke(cli, ["--version"]) assert result.exit_code == 0 - assert "0.5.3" in result.output + assert "0.7.3" in result.output def test_no_command_shows_usage(self, runner): result = runner.invoke(cli, []) diff --git a/tools/validate-docs.py b/tools/validate-docs.py index 0ab3766..79780fd 100755 --- a/tools/validate-docs.py +++ b/tools/validate-docs.py @@ -122,7 +122,9 @@ def read_file_lines(file_path: Path) -> list[str] | None: return None -def check_hardcoded_counts(lines: list[str], shipped_count: int, file_path: Path) -> list[str]: +def check_hardcoded_counts( + lines: list[str], shipped_count: int, total_count: int, file_path: Path, +) -> list[str]: """Flag lines with hardcoded critic counts that don't match shipped count.""" findings: list[str] = [] # Match patterns like "4 critics", "ships with 4", "currently 5 critics" @@ -165,7 +167,7 @@ def check_hardcoded_counts(lines: list[str], shipped_count: int, file_path: Path n = int(match.group(1)) # Only flag if it looks like a shipped count that's wrong # Skip 9 (total architecture), 3 (planned), and the correct count - if n != shipped_count and n in range(2, 9) and n != 9: + if n != shipped_count and n != total_count and 2 <= n <= total_count: # Skip lines about specific depth profiles with intentionally fewer critics # (e.g., "quick" depth legitimately runs 2 critics) if "quick" in line_lower and n == 2: @@ -263,33 +265,40 @@ def check_roadmap_shipped( return findings +def _extract_pyproject_version(content: str) -> str | None: + """Extract the version string from pyproject.toml content (pure).""" + match = re.search(r'^version\s*=\s*"([^"]+)"', content, re.MULTILINE) + return match.group(1) if match else None + + +def _extract_badge_versions(content: str) -> list[str]: + """Extract version strings from badge markup in README content (pure).""" + badge_pattern = re.compile(r'badge[/](?:version-)?v?(\d+\.\d+\.\d+)') + return [m.group(1) for m in badge_pattern.finditer(content)] + + def check_version_consistency(repo_root: Path, manifest_version: str) -> list[str]: """Check that version strings across the repo match the manifest version.""" findings: list[str] = [] - # Check pyproject.toml pyproject = repo_root / "reference-implementation" / "pyproject.toml" if pyproject.exists(): try: content = pyproject.read_text(encoding="utf-8") - version_match = re.search(r'^version\s*=\s*"([^"]+)"', content, re.MULTILINE) - if version_match and version_match.group(1) != manifest_version: + pyproject_ver = _extract_pyproject_version(content) + if pyproject_ver and pyproject_ver != manifest_version: findings.append( - f" pyproject.toml: version=\"{version_match.group(1)}\" " + f" pyproject.toml: version=\"{pyproject_ver}\" " f"(manifest={manifest_version})" ) except OSError: pass - # Check README for badge version mismatches readme = repo_root / "README.md" if readme.exists(): try: content = readme.read_text(encoding="utf-8") - # Match badge patterns like: badge/version-v0.5.3 or badge/v0.5.3 - badge_pattern = re.compile(r'badge[/](?:version-)?v?(\d+\.\d+\.\d+)') - for match in badge_pattern.finditer(content): - badge_ver = match.group(1) + for badge_ver in _extract_badge_versions(content): if badge_ver != manifest_version: findings.append( f" README.md: badge version \"{badge_ver}\" " @@ -396,6 +405,7 @@ def validate_docs(repo_root: Path) -> list[str]: manifest = load_manifest(repo_root) shipped = get_critics_by_status(manifest, "shipped") shipped_count = len(shipped) + total_count = len(manifest.get("critics", {})) manifest_version = manifest.get("version", "unknown") md_files = find_md_files(repo_root) @@ -414,7 +424,7 @@ def validate_docs(repo_root: Path) -> list[str]: continue rel_path = md_file.relative_to(repo_root) - all_findings.extend(check_hardcoded_counts(lines, shipped_count, rel_path)) + all_findings.extend(check_hardcoded_counts(lines, shipped_count, total_count, rel_path)) all_findings.extend(check_stale_status_markers(lines, shipped, rel_path)) all_findings.extend(check_roadmap_shipped(lines, shipped, rel_path)) all_findings.extend(check_depth_config_claims(lines, manifest, rel_path)) @@ -429,10 +439,10 @@ def main() -> int: script_dir = Path(__file__).resolve().parent repo_root = script_dir.parent + # Print manifest summary for human readers manifest = load_manifest(repo_root) shipped = get_critics_by_status(manifest, "shipped") planned = get_critics_by_status(manifest, "planned") - shipped_count = len(shipped) manifest_version = manifest.get("version", "unknown") callable_critics = { @@ -442,7 +452,7 @@ def main() -> int: depth_configs = manifest.get("depth_configs", {}) print(f"Manifest version: {manifest_version}") - print(f"Shipped critics ({shipped_count}): {', '.join(shipped.keys())}") + print(f"Shipped critics ({len(shipped)}): {', '.join(shipped.keys())}") print(f"Callable critics ({len(callable_critics)}): {', '.join(sorted(callable_critics))}") print(f"Planned critics ({len(planned)}): {', '.join(planned.keys())}") if depth_configs: @@ -453,25 +463,8 @@ def main() -> int: md_files = find_md_files(repo_root) print(f"Scanning {len(md_files)} markdown files...\n") - all_findings: list[str] = [] - - # Version consistency check - version_findings = check_version_consistency(repo_root, manifest_version) - if version_findings: - all_findings.append("VERSION MISMATCH:") - all_findings.extend(version_findings) - - for md_file in md_files: - lines = read_file_lines(md_file) - if lines is None: - continue - - rel_path = md_file.relative_to(repo_root) - all_findings.extend(check_hardcoded_counts(lines, shipped_count, rel_path)) - all_findings.extend(check_stale_status_markers(lines, shipped, rel_path)) - all_findings.extend(check_roadmap_shipped(lines, shipped, rel_path)) - all_findings.extend(check_depth_config_claims(lines, manifest, rel_path)) - all_findings.extend(check_framework_version_strings(lines, manifest_version, rel_path)) + # Delegate validation logic (no duplication with validate_docs()) + all_findings = validate_docs(repo_root) if all_findings: print(f"FINDINGS ({len(all_findings)}):\n")