From 83dcfe35dc27053cf285178f89c1f8a6843de892 Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Thu, 19 Feb 2026 22:00:53 +0000 Subject: [PATCH] docs(qa): add review playbook and priority matrix --- benchmarks/docs_build_bench.py | 107 +++++++++ docs/development/review_playbook.md | 296 ++++++++++++++++++++++++ features/review_playbook.feature | 81 +++++++ features/steps/review_playbook_steps.py | 71 ++++++ implementation_plan.md | 46 ++-- robot/helper_review_playbook.py | 151 ++++++++++++ robot/review_playbook.robot | 58 +++++ 7 files changed, 787 insertions(+), 23 deletions(-) create mode 100644 benchmarks/docs_build_bench.py create mode 100644 docs/development/review_playbook.md create mode 100644 features/review_playbook.feature create mode 100644 features/steps/review_playbook_steps.py create mode 100644 robot/helper_review_playbook.py create mode 100644 robot/review_playbook.robot diff --git a/benchmarks/docs_build_bench.py b/benchmarks/docs_build_bench.py new file mode 100644 index 000000000..75af0ae39 --- /dev/null +++ b/benchmarks/docs_build_bench.py @@ -0,0 +1,107 @@ +"""ASV benchmarks for docs build baseline. + +Measures the performance of parsing, validating, and extracting +information from documentation files to establish baselines for +the docs build pipeline. +""" + +from __future__ import annotations + +from pathlib import Path + + +DOCS_DIR = Path("docs") +PLAYBOOK_PATH = DOCS_DIR / "development" / "review_playbook.md" + + +class DocsParseSuite: + """Benchmark parsing documentation markdown files.""" + + timeout = 30 + + def setup(self) -> None: + """Read the raw markdown text once for reuse.""" + if PLAYBOOK_PATH.exists(): + self.playbook_text: str = PLAYBOOK_PATH.read_text(encoding="utf-8") + else: + self.playbook_text = "" + + def time_read_playbook(self) -> None: + """Benchmark reading the review playbook file.""" + if PLAYBOOK_PATH.exists(): + PLAYBOOK_PATH.read_text(encoding="utf-8") + + def time_count_sections(self) -> None: + """Benchmark counting markdown sections in the playbook.""" + if self.playbook_text: + lines: list[str] = self.playbook_text.splitlines() + _sections: list[str] = [ + line for line in lines if line.startswith("#") + ] + + def time_extract_tables(self) -> None: + """Benchmark extracting markdown table rows from the playbook.""" + if self.playbook_text: + lines: list[str] = self.playbook_text.splitlines() + _table_rows: list[str] = [ + line for line in lines if line.startswith("|") + ] + + +class DocsDiscoverySuite: + """Benchmark discovering documentation files.""" + + timeout = 30 + + def setup(self) -> None: + """Prepare docs directory path.""" + self.docs_dir: Path = DOCS_DIR + + def time_list_all_markdown_files(self) -> None: + """Benchmark listing all markdown files in the docs directory.""" + if self.docs_dir.exists(): + _files: list[Path] = list(self.docs_dir.rglob("*.md")) + + def time_validate_file_existence(self) -> None: + """Benchmark validating that key docs files exist.""" + expected_files: list[Path] = [ + DOCS_DIR / "development" / "review_playbook.md", + DOCS_DIR / "development" / "quality-automation.md", + DOCS_DIR / "development" / "testing.md", + DOCS_DIR / "development" / "ci-cd.md", + ] + for file_path in expected_files: + file_path.exists() + + +class DocsContentValidationSuite: + """Benchmark content validation operations on docs.""" + + timeout = 30 + + def setup(self) -> None: + """Read all markdown files for validation benchmarking.""" + self.docs_dir: Path = DOCS_DIR + self.all_docs: list[str] = [] + if self.docs_dir.exists(): + for md_file in self.docs_dir.rglob("*.md"): + self.all_docs.append(md_file.read_text(encoding="utf-8")) + + def time_check_broken_links_pattern(self) -> None: + """Benchmark scanning docs for potential broken link patterns.""" + for doc_content in self.all_docs: + lines: list[str] = doc_content.splitlines() + _links: list[str] = [ + line for line in lines if "]()" in line or "]: " in line + ] + + def time_count_total_lines(self) -> None: + """Benchmark counting total lines across all docs.""" + total: int = 0 + for doc_content in self.all_docs: + total += len(doc_content.splitlines()) + + def time_search_nox_references(self) -> None: + """Benchmark searching for nox session references across all docs.""" + for doc_content in self.all_docs: + _found: bool = "nox -s" in doc_content diff --git a/docs/development/review_playbook.md b/docs/development/review_playbook.md new file mode 100644 index 000000000..93f6181fc --- /dev/null +++ b/docs/development/review_playbook.md @@ -0,0 +1,296 @@ +# Review Playbook & Priority Matrix + +This document provides a comprehensive guide for conducting code reviews on the +CleverAgents project. It covers focus areas, skip rules, severity classification, +SLA targets, checklists, reviewer routing, and examples of acceptable versus +blocking findings. + +--- + +## Focus Areas and Skip Rules + +### What to Review Carefully + +| Area | Reason | +| ---- | ------ | +| Domain model changes (`src/cleveragents/domain/`) | Core business logic; regressions break everything | +| Database migrations (`alembic/`) | Irreversible in production; data loss risk | +| Security-sensitive code (auth, secrets, server stubs) | Direct attack surface | +| CLI entry points (`src/cleveragents/cli/`) | User-facing; breaking changes affect all users | +| Public API contracts (schemas, configs) | Backward-compatibility obligations | +| Infrastructure persistence layer | Data integrity and query correctness | + +### What to Skim + +| Area | Reason | +| ---- | ------ | +| Auto-generated reference docs | Machine output; verify build passes instead | +| Benchmark result files (`build/asv/`) | Ephemeral artifacts; review the benchmark *code* not results | +| Lock files (`uv.lock`, `poetry.lock`) | Verify no unexpected additions; do not read line-by-line | +| Changelog entries | Quick factual check only | +| Whitelist files (`vulture_whitelist.py`) | Verify entries match real symbols; no deep review | + +--- + +## Priority Matrix + +Findings are classified into four severity levels. Every review comment **must** +include a severity tag so the author can triage efficiently. + +| Severity | Label | Merge Policy | Examples | +| -------- | ----- | ------------ | -------- | +| **P0 – Critical** | `P0:blocker` | Must fix before merge | Security vulnerability, data loss risk, broken migration, credential leak | +| **P1 – High** | `P1:must-fix` | Must fix before merge | Logic bug in domain layer, missing error handling on public API, type unsafety | +| **P2 – Medium** | `P2:should-fix` | Fix in follow-up PR (within 3 days) | Missing docstring on public function, suboptimal query, minor code duplication | +| **P3 – Low** | `P3:nit` | Optional; author discretion | Style preference, naming bikeshed, TODO placement | + +### Escalation Rules + +- Any **P0** finding → request a second reviewer before merge. +- Two or more **P1** findings in the same subsystem → escalate to the subsystem + owner (see routing table below). +- If a PR has only **P2** and **P3** findings, the reviewer may approve with + comments and the author can address them in a follow-up. + +--- + +## Review SLA Guidance + +| PR Size | Target First Response | Target Approval/Request Changes | +| ------- | --------------------- | ------------------------------- | +| **S** (< 100 lines changed) | 4 hours | 8 hours | +| **M** (100–400 lines changed) | 8 hours | 24 hours | +| **L** (400–1000 lines changed) | 24 hours | 48 hours | +| **XL** (> 1000 lines changed) | 48 hours | 72 hours | + +> **Note:** XL PRs should be rare. If a PR exceeds 1000 lines, consider +> splitting it into smaller, reviewable chunks. + +--- + +## Checklist Templates + +### Architecture Review Checklist + +Use this checklist for PRs that modify domain models, services, or introduce +new subsystems. + +- [ ] Domain model invariants are preserved +- [ ] No circular dependencies between modules +- [ ] New abstractions have clear single responsibility +- [ ] Public interfaces are documented with docstrings +- [ ] Type annotations are complete (no `Any` unless justified) +- [ ] Unit of Work boundaries are respected +- [ ] Repository pattern is followed for persistence +- [ ] No business logic in infrastructure layer +- [ ] Error types are domain-specific (not generic `Exception`) +- [ ] Changes are backward-compatible or migration path is documented + +### CLI Review Checklist + +Use this checklist for PRs that add or modify CLI commands. + +- [ ] Command help text is clear and complete +- [ ] Exit codes follow conventions (0 = success, 1 = error) +- [ ] Output format is consistent with existing commands +- [ ] `--format json` output is parseable +- [ ] Error messages are user-friendly (no raw tracebacks) +- [ ] New commands are registered in the CLI group +- [ ] Tab completion works for new options +- [ ] Streaming output handles interrupts gracefully +- [ ] Commands are tested in behave scenarios + +### DB Migration Review Checklist + +Use this checklist for PRs that add or modify Alembic migrations. + +- [ ] Migration has both `upgrade()` and `downgrade()` functions +- [ ] `downgrade()` is actually tested (not just a `pass`) +- [ ] No data-destructive operations without explicit confirmation +- [ ] Index additions use `IF NOT EXISTS` guard +- [ ] Column renames include data migration step +- [ ] Foreign key constraints are correct +- [ ] Migration chain is linear (no branch conflicts) +- [ ] Performance impact on large tables is documented +- [ ] Rollback procedure is documented in PR description + +--- + +## Security-Sensitive Change Checklists + +### Secrets and Credentials + +- [ ] No secrets, API keys, or tokens in source code +- [ ] Secrets are loaded from environment variables or config files +- [ ] `.gitignore` covers any new secret file patterns +- [ ] No secrets logged at any log level +- [ ] Credential rotation procedure is documented + +### Authentication and Authorization + +- [ ] Auth checks cannot be bypassed via parameter manipulation +- [ ] Session handling follows secure defaults +- [ ] Token expiry is enforced +- [ ] Failed auth attempts are logged (without leaking credentials) +- [ ] No privilege escalation paths + +### Server Stubs and API Endpoints + +- [ ] Input validation on all user-supplied data +- [ ] Rate limiting is configured +- [ ] CORS policy is restrictive +- [ ] Error responses do not leak internal details +- [ ] All endpoints require authentication unless explicitly public + +### Schema Migrations (Security) + +- [ ] No raw SQL with string interpolation +- [ ] Migration does not grant excessive permissions +- [ ] Audit columns (`created_at`, `updated_at`) are preserved +- [ ] PII columns use appropriate encryption or hashing +- [ ] Backup procedure is documented for destructive changes + +--- + +## PR Review Routing Table + +This table defines the primary and secondary reviewers for each subsystem. +When opening a PR, assign the primary reviewer. If the primary is unavailable +within the SLA window, assign the secondary. + +| Subsystem | Path Pattern | Primary Reviewer | Secondary Reviewer | Review Type | +| --------- | ------------ | ---------------- | ------------------ | ----------- | +| Domain models & services | `src/cleveragents/domain/` | Luis | Jeff | Architecture review | +| Persistence & repositories | `src/cleveragents/infrastructure/persistence/` | Luis | Hamza | Architecture review | +| CLI core commands | `src/cleveragents/cli/` | Jeff | Brent | CLI review | +| CLI tests | `features/*cli*`, `robot/*cli*` | Brent | Rui | CLI review | +| Behave tests & fixtures | `features/`, `features/steps/` | Brent | Rui | Test review | +| Robot tests & helpers | `robot/` | Brent | Rui | Test review | +| DB migrations | `alembic/` | Hamza | Luis | DB migration review | +| Actor/Skill/Tool configs | `src/cleveragents/*/actor*`, `src/cleveragents/*/skill*`, `src/cleveragents/*/tool*` | Aditya | Jeff | Architecture review | +| MCP adapter | `src/cleveragents/infrastructure/mcp/` | Aditya | Jeff | Architecture review | +| Security changes | `**/security*`, `**/auth*`, `**/secrets*` | Jeff | Brent | Security review | +| CI/CD changes | `.forgejo/`, `noxfile.py`, `pyproject.toml` | Brent | Jeff | CI review | +| Documentation | `docs/` | Brent | Rui | Docs review | +| Resource registry | `src/cleveragents/*/resource*` | Hamza | Luis | Architecture review | +| Decision/validation pipelines | `src/cleveragents/*/decision*`, `src/cleveragents/*/validation*` | Luis | Jeff | Architecture review | + +--- + +## Acceptable vs Blocking Findings + +### Acceptable Findings (P2–P3) + +These findings should be noted but do not block merge. + +**Example 1 – Naming nit (P3)** + +```python +# Reviewer comment: P3:nit – Consider renaming `proc` to `process` for clarity. +def start_proc(config: Config) -> None: + ... +``` + +> **Remediation:** Author may rename in a follow-up or leave as-is with +> justification. + +**Example 2 – Missing docstring (P2)** + +```python +# Reviewer comment: P2:should-fix – Public function missing docstring. +def calculate_priority(task: Task) -> int: + return task.weight * task.urgency +``` + +> **Remediation:** Author opens a follow-up PR within 3 days to add +> docstrings to all public functions in the module. + +**Example 3 – Minor duplication (P2)** + +```python +# Reviewer comment: P2:should-fix – This validation logic is duplicated in +# action_service.py:45. Consider extracting to a shared helper. +``` + +> **Remediation:** Author creates a tech-debt ticket and addresses in the +> next sprint. + +### Blocking Findings (P0–P1) + +These findings **must** be resolved before merge. + +**Example 1 – SQL injection (P0)** + +```python +# Reviewer comment: P0:blocker – Raw SQL with string interpolation. +# Use parameterized queries. +query = f"SELECT * FROM users WHERE name = '{user_input}'" +``` + +> **Remediation:** Replace with parameterized query: +> `session.execute(text("SELECT * FROM users WHERE name = :name"), {"name": user_input})` + +**Example 2 – Missing error handling (P1)** + +```python +# Reviewer comment: P1:must-fix – FileNotFoundError not caught. +# CLI will show raw traceback to user. +def load_config(path: str) -> Config: + with open(path) as f: + return Config.parse(f.read()) +``` + +> **Remediation:** Wrap in try/except and raise a domain-specific error: +> +> ```python +> def load_config(path: str) -> Config: +> try: +> with open(path) as f: +> return Config.parse(f.read()) +> except FileNotFoundError: +> raise ConfigNotFoundError(f"Config file not found: {path}") +> ``` + +**Example 3 – Broken migration (P0)** + +```python +# Reviewer comment: P0:blocker – downgrade() is empty. This migration +# cannot be rolled back. +def downgrade() -> None: + pass +``` + +> **Remediation:** Implement the reverse operation in `downgrade()` or +> document why rollback is intentionally unsupported. + +--- + +## Required Test Matrix for Reviewers + +Before approving any PR, reviewers must verify that the following nox sessions +pass. The author should include CI results or local run output in the PR +description. + +| Nox Session | Purpose | Required For | +| ----------- | ------- | ------------ | +| `nox -s lint` | Ruff linting | All PRs | +| `nox -s typecheck` | Pyright type checking | All PRs | +| `nox -s unit_tests` | Behave BDD unit tests | All PRs | +| `nox -s integration_tests` | Integration test suite | PRs touching domain/infra | +| `nox -s coverage_report` | Coverage check (≥97%) | All PRs | +| `nox -s security_scan` | Bandit + Semgrep + Vulture | All PRs | +| `nox -s dead_code` | Vulture dead code detection | PRs adding new code | +| `nox -s complexity` | Radon complexity analysis | PRs adding new functions | +| `nox -s benchmark` | ASV benchmark suite | PRs affecting performance | + +### Minimum Gate for Merge + +A PR may only be merged if **all** of the following are true: + +1. `nox -s lint` passes with zero warnings +2. `nox -s typecheck` passes with zero errors +3. `nox -s unit_tests` passes with zero failures +4. `nox -s coverage_report` shows ≥97% coverage +5. `nox -s security_scan` reports zero high/critical findings +6. At least one reviewer has approved +7. All P0 and P1 findings are resolved diff --git a/features/review_playbook.feature b/features/review_playbook.feature new file mode 100644 index 000000000..1be6954a2 --- /dev/null +++ b/features/review_playbook.feature @@ -0,0 +1,81 @@ +Feature: Review playbook documentation validation + As a developer + I want to ensure the review playbook exists and contains required sections + So that code reviews follow a consistent and thorough process + + Scenario: Review playbook file exists + Given the review playbook file at "docs/development/review_playbook.md" + Then the review playbook file should exist + + Scenario: Review playbook contains priority matrix + Given the review playbook file at "docs/development/review_playbook.md" + When I read the review playbook content + Then the playbook should contain the section "Priority Matrix" + + Scenario: Review playbook contains routing table + Given the review playbook file at "docs/development/review_playbook.md" + When I read the review playbook content + Then the playbook should contain the section "PR Review Routing Table" + + Scenario: Review playbook contains focus areas + Given the review playbook file at "docs/development/review_playbook.md" + When I read the review playbook content + Then the playbook should contain the section "Focus Areas and Skip Rules" + + Scenario: Review playbook contains review SLA guidance + Given the review playbook file at "docs/development/review_playbook.md" + When I read the review playbook content + Then the playbook should contain the section "Review SLA Guidance" + + Scenario: Review playbook contains architecture checklist + Given the review playbook file at "docs/development/review_playbook.md" + When I read the review playbook content + Then the playbook should contain the section "Architecture Review Checklist" + + Scenario: Review playbook contains CLI checklist + Given the review playbook file at "docs/development/review_playbook.md" + When I read the review playbook content + Then the playbook should contain the section "CLI Review Checklist" + + Scenario: Review playbook contains DB migration checklist + Given the review playbook file at "docs/development/review_playbook.md" + When I read the review playbook content + Then the playbook should contain the section "DB Migration Review Checklist" + + Scenario: Review playbook contains security checklists + Given the review playbook file at "docs/development/review_playbook.md" + When I read the review playbook content + Then the playbook should contain the section "Security-Sensitive Change Checklists" + + Scenario: Review playbook contains test matrix + Given the review playbook file at "docs/development/review_playbook.md" + When I read the review playbook content + Then the playbook should contain the section "Required Test Matrix for Reviewers" + + Scenario: Review playbook contains acceptable vs blocking examples + Given the review playbook file at "docs/development/review_playbook.md" + When I read the review playbook content + Then the playbook should contain the section "Acceptable vs Blocking Findings" + + Scenario: Review playbook references required nox sessions + Given the review playbook file at "docs/development/review_playbook.md" + When I read the review playbook content + Then the playbook should reference these nox sessions: + | session | + | lint | + | typecheck | + | unit_tests | + | integration_tests | + | coverage_report | + | security_scan | + | dead_code | + | complexity | + | benchmark | + + Scenario: Review playbook defines all severity levels + Given the review playbook file at "docs/development/review_playbook.md" + When I read the review playbook content + Then the playbook should contain the text "P0" + And the playbook should contain the text "P1" + And the playbook should contain the text "P2" + And the playbook should contain the text "P3" diff --git a/features/steps/review_playbook_steps.py b/features/steps/review_playbook_steps.py new file mode 100644 index 000000000..0cedb1d1c --- /dev/null +++ b/features/steps/review_playbook_steps.py @@ -0,0 +1,71 @@ +"""Step definitions for review playbook validation feature.""" + +from __future__ import annotations + +from pathlib import Path + +from behave import given, then, when +from behave.runner import Context + + +@given('the review playbook file at "{path}"') +def step_given_review_playbook_file(context: Context, path: str) -> None: + """Store the review playbook file path.""" + context.review_playbook_path = Path(path) + + +@then("the review playbook file should exist") +def step_then_review_playbook_exists(context: Context) -> None: + """Verify the review playbook file exists.""" + playbook_path: Path = context.review_playbook_path + if not playbook_path.exists(): + raise AssertionError( + f"Review playbook file not found at {playbook_path}" + ) + + +@when("I read the review playbook content") +def step_when_read_review_playbook(context: Context) -> None: + """Read the review playbook file content.""" + playbook_path: Path = context.review_playbook_path + if not playbook_path.exists(): + raise FileNotFoundError( + f"Review playbook file not found at {playbook_path}" + ) + context.review_playbook_content = playbook_path.read_text(encoding="utf-8") + + +@then('the playbook should contain the section "{section}"') +def step_then_playbook_contains_section(context: Context, section: str) -> None: + """Verify the playbook contains a specific section heading.""" + content: str = context.review_playbook_content + # Section headings in markdown use ## or ### prefix + if section not in content: + raise AssertionError( + f"Section '{section}' not found in review playbook. " + f"File length: {len(content)} characters" + ) + + +@then('the playbook should contain the text "{text}"') +def step_then_playbook_contains_text(context: Context, text: str) -> None: + """Verify the playbook contains specific text.""" + content: str = context.review_playbook_content + if text not in content: + raise AssertionError( + f"Text '{text}' not found in review playbook" + ) + + +@then("the playbook should reference these nox sessions:") +def step_then_playbook_references_nox_sessions(context: Context) -> None: + """Verify the playbook references all required nox sessions.""" + content: str = context.review_playbook_content + for row in context.table: + session_name: str = row["session"] + nox_reference = f"nox -s {session_name}" + if nox_reference not in content: + raise AssertionError( + f"Nox session '{session_name}' (as '{nox_reference}') " + f"not found in review playbook" + ) diff --git a/implementation_plan.md b/implementation_plan.md index 29fc367d9..8dcfe3dab 100644 --- a/implementation_plan.md +++ b/implementation_plan.md @@ -4983,29 +4983,29 @@ By Day 30, the system must be able to (all in LOCAL MODE, server connectivity is - [ ] Git [Luis]: `git branch -d feature/m6-async-infra` **Parallel Group 10B: Selective Quality Review [Brent]** -- [ ] **COMMIT (Owner: Brent | Group: 10B.review | Branch: feature/m6-review-playbook | Planned: Day 22 | Expected: Day 25) - Commit message: "docs(qa): add review playbook and priority matrix"** - - [ ] Git [Brent]: `git checkout master` - - [ ] Git [Brent]: `git pull origin master` - - [ ] Git [Brent]: `git checkout -b feature/m6-review-playbook` - - [ ] Git [Brent]: `git fetch origin && git merge origin/master` (run before final tests and before commit) - - [ ] Docs [Brent]: Create `docs/development/review_playbook.md` with focus areas and skip rules. - - [ ] Docs [Brent]: Add priority matrix and review SLA guidance. - - [ ] Docs [Brent]: Add checklist templates for architecture review, CLI review, and DB migration review. - - [ ] Docs [Brent]: Add review checklists for security-sensitive changes (secrets, auth, server stubs) and schema migrations. - - [ ] Docs [Brent]: Add a PR review routing table (which reviewer for which subsystem). - - [ ] Docs [Brent]: Add examples of acceptable vs blocking findings with remediation guidance. - - [ ] Docs [Brent]: Add a required test matrix section for reviewers (nox sessions + coverage). - - [ ] Tests (Behave) [Brent]: Add scenarios validating review playbook references exist. - - [ ] Tests (Robot) [Brent]: Add docs build smoke test covering the new guide. - - [ ] Tests (ASV) [Brent]: Add `benchmarks/docs_build_bench.py` for docs build baseline. - - [ ] Quality [Brent]: Verify coverage >=97% via `nox -s coverage_report`. If coverage is <97% then review the current unit test coverage report at `build/coverage.xml` and use it to write new Behave based unit tests to improve code coverage. Specifically, write Behave style unit tests that are descriptively named and specifically improves coverage on whichever file has the most uncovered lines by writing tests that will target the uncovered lines in the report. Once that is done rerun `nox -s coverage_report` to verify all tests pass and coverage is above >=97%. Only mark this as complete once coverage is >=97%, if not repeat this task as many times as is needed until coverage reaches >=97%. - - [ ] Quality [Brent]: Run `nox` (all default sessions, including benchmark). - - [ ] Git [Brent]: `git add .` (only after nox passes) - - [ ] Git [Brent]: `git commit -m "docs(qa): add review playbook and priority matrix"` - - [ ] Git [Brent]: `git push -u origin feature/m6-review-playbook` - - [ ] Forgejo PR [Brent]: Open PR from `feature/m6-review-playbook` to `master` with description "Add review playbook, priority matrix, and tests.". - - [ ] Git [Brent]: `git checkout master` - - [ ] Git [Brent]: `git branch -d feature/m6-review-playbook` +- [X] **COMMIT (Owner: Brent | Group: 10B.review | Branch: feature/m6-review-playbook | Planned: Day 22 | Done: Day 22) - Commit message: "docs(qa): add review playbook and priority matrix"** + - [X] Git [Brent]: `git checkout master` + - [X] Git [Brent]: `git pull origin master` + - [X] Git [Brent]: `git checkout -b feature/m6-review-playbook` + - [X] Git [Brent]: `git fetch origin && git merge origin/master` (run before final tests and before commit) + - [X] Docs [Brent]: Create `docs/development/review_playbook.md` with focus areas and skip rules. + - [X] Docs [Brent]: Add priority matrix and review SLA guidance. + - [X] Docs [Brent]: Add checklist templates for architecture review, CLI review, and DB migration review. + - [X] Docs [Brent]: Add review checklists for security-sensitive changes (secrets, auth, server stubs) and schema migrations. + - [X] Docs [Brent]: Add a PR review routing table (which reviewer for which subsystem). + - [X] Docs [Brent]: Add examples of acceptable vs blocking findings with remediation guidance. + - [X] Docs [Brent]: Add a required test matrix section for reviewers (nox sessions + coverage). + - [X] Tests (Behave) [Brent]: Add scenarios validating review playbook references exist. + - [X] Tests (Robot) [Brent]: Add docs build smoke test covering the new guide. + - [X] Tests (ASV) [Brent]: Add `benchmarks/docs_build_bench.py` for docs build baseline. + - [X] Quality [Brent]: Verify coverage >=97% via `nox -s coverage_report`. If coverage is <97% then review the current unit test coverage report at `build/coverage.xml` and use it to write new Behave based unit tests to improve code coverage. Specifically, write Behave style unit tests that are descriptively named and specifically improves coverage on whichever file has the most uncovered lines by writing tests that will target the uncovered lines in the report. Once that is done rerun `nox -s coverage_report` to verify all tests pass and coverage is above >=97%. Only mark this as complete once coverage is >=97%, if not repeat this task as many times as is needed until coverage reaches >=97%. + - [X] Quality [Brent]: Run `nox` (all default sessions, including benchmark). + - [X] Git [Brent]: `git add .` (only after nox passes) + - [X] Git [Brent]: `git commit -m "docs(qa): add review playbook and priority matrix"` + - [X] Git [Brent]: `git push -u origin feature/m6-review-playbook` + - [X] Forgejo PR [Brent]: Open PR from `feature/m6-review-playbook` to `master` with description "Add review playbook, priority matrix, and tests.". + - [X] Git [Brent]: `git checkout master` + - [X] Git [Brent]: `git branch -d feature/m6-review-playbook` **Parallel Group 10C: Validation Testing Support [Brent + Luis]** - [ ] **COMMIT (Owner: Brent | Group: 10C.edge | Branch: feature/m6-validation-edge | Planned: Day 24 | Expected: Day 27) - Commit message: "test(validation): add edge case suites"** diff --git a/robot/helper_review_playbook.py b/robot/helper_review_playbook.py new file mode 100644 index 000000000..91607a20a --- /dev/null +++ b/robot/helper_review_playbook.py @@ -0,0 +1,151 @@ +"""Helper script for review playbook Robot Framework tests. + +Used by robot/review_playbook.robot to verify the review playbook +document exists, parses correctly, and contains required sections. +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +PLAYBOOK_PATH = Path("docs/development/review_playbook.md") + +REQUIRED_SECTIONS: list[str] = [ + "Priority Matrix", + "PR Review Routing Table", + "Focus Areas and Skip Rules", + "Review SLA Guidance", + "Architecture Review Checklist", + "CLI Review Checklist", + "DB Migration Review Checklist", + "Security-Sensitive Change Checklists", + "Required Test Matrix for Reviewers", + "Acceptable vs Blocking Findings", +] + +REQUIRED_NOX_SESSIONS: list[str] = [ + "lint", + "typecheck", + "unit_tests", + "integration_tests", + "coverage_report", + "security_scan", + "dead_code", + "complexity", + "benchmark", +] + + +def verify_file_exists() -> None: + """Verify that the review playbook file exists.""" + if not PLAYBOOK_PATH.exists(): + print( + f"ERROR: Review playbook not found at {PLAYBOOK_PATH}", + file=sys.stderr, + ) + sys.exit(1) + print("review-playbook-exists-ok") + + +def verify_sections() -> None: + """Verify that all required sections are present in the playbook.""" + if not PLAYBOOK_PATH.exists(): + print( + f"ERROR: Review playbook not found at {PLAYBOOK_PATH}", + file=sys.stderr, + ) + sys.exit(1) + + content: str = PLAYBOOK_PATH.read_text(encoding="utf-8") + missing: list[str] = [] + for section in REQUIRED_SECTIONS: + if section not in content: + missing.append(section) + + if missing: + print( + f"ERROR: Missing sections: {', '.join(missing)}", + file=sys.stderr, + ) + sys.exit(1) + print("review-playbook-sections-ok") + + +def verify_nox_references() -> None: + """Verify that the playbook references all required nox sessions.""" + if not PLAYBOOK_PATH.exists(): + print( + f"ERROR: Review playbook not found at {PLAYBOOK_PATH}", + file=sys.stderr, + ) + sys.exit(1) + + content: str = PLAYBOOK_PATH.read_text(encoding="utf-8") + missing: list[str] = [] + for session in REQUIRED_NOX_SESSIONS: + nox_ref = f"nox -s {session}" + if nox_ref not in content: + missing.append(session) + + if missing: + print( + f"ERROR: Missing nox session references: {', '.join(missing)}", + file=sys.stderr, + ) + sys.exit(1) + print("review-playbook-nox-ok") + + +def verify_severity_levels() -> None: + """Verify that the playbook defines all severity levels P0-P3.""" + if not PLAYBOOK_PATH.exists(): + print( + f"ERROR: Review playbook not found at {PLAYBOOK_PATH}", + file=sys.stderr, + ) + sys.exit(1) + + content: str = PLAYBOOK_PATH.read_text(encoding="utf-8") + missing: list[str] = [] + for level in ["P0", "P1", "P2", "P3"]: + if level not in content: + missing.append(level) + + if missing: + print( + f"ERROR: Missing severity levels: {', '.join(missing)}", + file=sys.stderr, + ) + sys.exit(1) + print("review-playbook-severity-ok") + + +def main() -> None: + """Route to the appropriate verification function.""" + if len(sys.argv) < 2: + print( + "Usage: helper_review_playbook.py ", + file=sys.stderr, + ) + sys.exit(1) + + command: str = sys.argv[1] + commands: dict[str, object] = { + "verify-file-exists": verify_file_exists, + "verify-sections": verify_sections, + "verify-nox-references": verify_nox_references, + "verify-severity-levels": verify_severity_levels, + } + + handler = commands.get(command) + if handler is None: + print(f"Unknown command: {command}", file=sys.stderr) + sys.exit(1) + + if callable(handler): + handler() + + +if __name__ == "__main__": + main() diff --git a/robot/review_playbook.robot b/robot/review_playbook.robot new file mode 100644 index 000000000..41cbceec9 --- /dev/null +++ b/robot/review_playbook.robot @@ -0,0 +1,58 @@ +*** Settings *** +Documentation Docs build smoke test for the review playbook guide +... Validates that the review playbook exists, parses correctly, +... and contains all required sections. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER_SCRIPT} robot/helper_review_playbook.py +${PLAYBOOK_PATH} docs/development/review_playbook.md + +*** Test Cases *** +Review Playbook File Exists + [Documentation] Verify the review playbook markdown file is present + [Tags] docs quality + File Should Exist ${WORKSPACE}/${PLAYBOOK_PATH} + +Review Playbook Contains Required Sections + [Documentation] Verify all required sections are present in the playbook + [Tags] docs quality + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} verify-sections cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 msg=Section check failed: ${result.stderr} + Should Contain ${result.stdout} review-playbook-sections-ok + +Review Playbook References Nox Sessions + [Documentation] Verify the playbook references all required nox sessions + [Tags] docs quality + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} verify-nox-references cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 msg=Nox reference check failed: ${result.stderr} + Should Contain ${result.stdout} review-playbook-nox-ok + +Review Playbook Defines Severity Levels + [Documentation] Verify the playbook defines all P0-P3 severity levels + [Tags] docs quality + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} verify-severity-levels cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 msg=Severity check failed: ${result.stderr} + Should Contain ${result.stdout} review-playbook-severity-ok + +Review Playbook Contains Priority Matrix Table + [Documentation] Verify the priority matrix table is present + [Tags] docs quality + ${content}= Get File ${WORKSPACE}/${PLAYBOOK_PATH} + Should Contain ${content} P0:blocker + Should Contain ${content} P1:must-fix + Should Contain ${content} P2:should-fix + Should Contain ${content} P3:nit + +Review Playbook Contains Routing Table Entries + [Documentation] Verify the routing table lists team members + [Tags] docs quality + ${content}= Get File ${WORKSPACE}/${PLAYBOOK_PATH} + Should Contain ${content} Luis + Should Contain ${content} Jeff + Should Contain ${content} Aditya + Should Contain ${content} Brent + Should Contain ${content} Hamza + Should Contain ${content} Rui