Add review-guidelines skill for structured code review process

Adds a new skill at agents/skills/review-guidelines/SKILL.md with an
8-step review checklist covering correctness, TAF pattern compliance,
conf file validation, security, code quality (trailing whitespace, debug
prints), and severity-tiered output (BLOCKER/MAJOR/MINOR/NIT). Wired
into .claude/skills/, .factory/skills/, settings.json, and AGENTS.md.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Change-Id: I8627176fa1685007e17c41cc5ff69e1c56594943
Reviewed-on: https://review.couchbase.org/c/TAF/+/245130
Reviewed-by: Bharath G P <bharath.gp@couchbase.com>
Tested-by: Ashwin <ashwin.govindarajulu@couchbase.com>

Author: ashwin2002

.claude/settings.json

@@ -9,6 +9,11 @@
       "name": "test_debugging",
       "description": "Given a scenario or test logs, maps to the actual code, debugs the failure, and proposes a concrete fix. Resolves which layer owns the failure, reads the source, diagnoses root cause, and provides before/after fix.",
       "path": "agents/skills/test_debugging.md"
+    },
+    {
+      "name": "review-guidelines",
+      "description": "Structured code review process for TAF changes. Use when asked to review a PR, diff, branch, or set of changed files — covers correctness, TAF pattern compliance, configuration validity, security, and produces prioritized BLOCKER/MAJOR/MINOR/NIT comments.",
+      "path": "agents/skills/review-guidelines/SKILL.md"
     }
   ]
 }

.claude/skills/review-guidelines/SKILL.md

@@ -0,0 +1 @@
+@agents/skills/review-guidelines/SKILL.md

.factory/skills/review-guidelines/SKILL.md

@@ -0,0 +1 @@
+@agents/skills/review-guidelines/SKILL.md

agents/skills/AGENTS.md

@@ -60,6 +60,23 @@ Comprehensive code map and flow reference for `pytests/upgrade/`. Covers class h
 
 ---
 
+### review-guidelines
+Structured code review process for TAF changes. Produces prioritized BLOCKER/MAJOR/MINOR/NIT comments across correctness, TAF patterns, configuration, security, and code quality.
+
+**Usage:** When asked to review a PR, diff, branch, or changed files — "review these changes", "check my changes before I commit".
+
+**Key features:**
+- 8-step review process: scope → correctness → TAF patterns → config → security → quality → docs → output
+- TAF-specific checks: base class, TestInputSingleton params, tearDown cleanup, submodule boundaries
+- Enforces `.conf` entry for every new `test_*` method added under `pytests/`
+- Catches leftover debug prints (`print`, `pdb`, `breakpoint`) and trailing whitespace
+- Severity-tiered output format (BLOCKER / MAJOR / MINOR / NIT) with file:line citations
+- Quick reference checklist for full coverage at a glance
+
+**File:** `agents/skills/review-guidelines/SKILL.md`
+
+---
+
 ## Adding a New Skill
 
 1. Create `agents/skills/<name>.md` with YAML frontmatter:

agents/skills/review-guidelines/SKILL.md

@@ -0,0 +1,231 @@
+---
+name: review-guidelines
+description: >-
+  Structured code review process for TAF changes. Provides step-by-step
+  guidance for reviewing Python test code, configuration files, and library
+  changes. Use when asked to review a PR, diff, or set of changed files in TAF.
+---
+
+# Agent Review Skill
+
+## Purpose
+
+Provide a structured, repeatable code review process tailored to TAF conventions.
+Covers correctness, test quality, framework patterns, configuration validity, and
+security — producing actionable, prioritized review comments.
+
+---
+
+## When to Use
+
+- "Review these changes"
+- "Check my changes before I commit"
+- "What's wrong with this code?"
+- Reviewing any changed `.py`, `.conf`, `.ini`, or `AGENTS.md` file in TAF
+
+---
+
+## Review Process
+
+### Step 1 — Understand the Scope
+
+Before reading any code:
+
+1. Run `git diff --stat` (or inspect the provided diff) to list changed files.
+2. Group files by layer:
+   - **Tests** — `pytests/**/*.py`
+   - **Libraries** — `lib/**/*.py`, `couchbase_utils/**/*.py`, `platform_utils/**/*.py`
+   - **Configuration** — `conf/**/*.conf`, `*.ini`
+   - **Documentation** — `agents/**/*.md`, `docs/**/*.md`, `AGENTS.md`
+3. Read the relevant `AGENTS.md` for each directory touched before opening `.py` files.
+4. State the summary: what feature/fix this change appears to implement.
+
+---
+
+### Step 2 — Correctness Review
+
+For each changed `.py` file:
+
+**Logic & Semantics**
+- [ ] Does the code do what the commit message / PR description claims?
+- [ ] Are all edge cases handled (empty collections, zero counts, `None` returns)?
+- [ ] Are conditional branches exhaustive — no implicit fall-throughs?
+- [ ] Are loop bounds correct (off-by-one, infinite loop risk)?
+
+**Error Handling**
+- [ ] Exceptions are caught at the right level — not swallowed silently.
+- [ ] Timeouts and retries are bounded; no unbounded `while True` without exit.
+- [ ] Failures surface a clear error message rather than a cryptic stack trace.
+
+**Data Integrity**
+- [ ] No mutable default arguments (`def foo(x=[])`).
+- [ ] Shared state (class variables, globals) is not accidentally mutated.
+- [ ] Thread/task safety for anything running in parallel task workers.
+
+---
+
+### Step 3 — TAF Pattern Compliance
+
+Check against TAF conventions (from `AGENTS.md`):
+
+**Naming**
+- [ ] Functions and variables: `snake_case`
+- [ ] Classes: `PascalCase`
+- [ ] Constants: `UPPER_SNAKE_CASE`
+- [ ] Test methods: `test_` prefix
+- [ ] Private helpers: `_leading_underscore`
+- Exception: `setUp`, `tearDown`, `setUpClass`, `tearDownClass` are allowed.
+
+**Test Structure**
+- [ ] Test class inherits from the correct base (`OnPremBaseTest`, `ColumnarBaseTest`, etc.) based on `runtype`.
+- [ ] Parameters accessed via `TestInputSingleton.input.param("name", default)` — no hard-coded values.
+- [ ] `tearDown` cleans up all resources created in the test.
+- [ ] No `print()` statements — use the framework logger.
+- [ ] No hard-coded cluster IPs, credentials, or cloud identifiers.
+
+**Document Loading**
+- [ ] If `load_docs_using` is referenced, valid values are `default_loader`, `sirius_java_sdk`, `sirius_go_sdk`.
+- [ ] `sirius_java_sdk` usage requires `--launch_java_doc_loader` flag documentation.
+
+**Submodule Boundaries**
+- [ ] No edits to `DocLoader/`, `lib/capellaAPI/`, or `sirius/` — these are maintained separately.
+
+---
+
+### Step 4 — Configuration File Review (`.conf` / `.ini`)
+
+For each changed `conf/**/*.conf`:
+
+- [ ] Each test entry follows the format: `module.class.test_method,param1=value1`
+- [ ] No duplicate test entries within the same file.
+- [ ] Parameter values are valid (no stray spaces, correct types).
+- [ ] If a new `test_*` method was added under `pytests/`, a corresponding `.conf` entry exists that exercises it.
+- [ ] If a test method was removed from `.py`, its `.conf` entry is also removed.
+- [ ] `.conf` entry for a new test is in the correct component conf file (matches the module path of the test class).
+
+For changed `*.ini` files:
+- [ ] No credentials or secrets committed.
+- [ ] Node topology is consistent (IP counts match role assignments).
+
+---
+
+### Step 5 — Security Review
+
+- [ ] No hard-coded passwords, API keys, tokens, or secrets anywhere.
+- [ ] No `shell=True` in `subprocess` calls with user-controlled input (command injection risk).
+- [ ] No `eval()` or `exec()` on untrusted data.
+- [ ] SSH commands use paramiko abstractions from `platform_utils/ssh_util/` — not raw shell concat.
+- [ ] REST API calls use existing `cb_server_rest_util` wrappers; no ad-hoc credential embedding in URLs.
+
+---
+
+### Step 6 — Code Quality & Maintainability
+
+**Readability**
+- [ ] Comments explain *why*, not *what* — no comment restates the code.
+- [ ] No commented-out dead code blocks left in.
+- [ ] No TODO/FIXME/HACK left without a linked ticket or rationale.
+- [ ] No trailing whitespace at end of any line (`grep -rn ' $' <file>` to verify).
+- [ ] No leftover debug statements — `print()`, `pdb.set_trace()`, `breakpoint()`, or temporary `logging.debug("here")`-style breadcrumbs (`grep -rn 'print\|pdb\|breakpoint' pytests/` to scan).
+
+**Duplication**
+- [ ] No copy-paste of existing utility methods — use `couchbase_utils/` helpers.
+- [ ] No re-implemented logic already in the base test class.
+- [ ] No repeated code blocks — extract to a shared helper in the base class or `couchbase_utils/`.
+
+**Size**
+- [ ] Functions are focused — no method doing 5 unrelated things.
+- [ ] Test methods test one scenario — split combined tests that assert unrelated outcomes.
+
+**Imports**
+- [ ] No unused imports.
+- [ ] Imports follow: stdlib → third-party → local framework.
+- [ ] No circular import dependencies — if module A imports B and B imports A, refactor shared logic into a third module (`pip install importlab` or `python -m py_compile` to surface cycles).
+
+---
+
+### Step 7 — Documentation & AGENTS.md
+
+- [ ] If a new class or public method was added, its `AGENTS.md` entry is updated (if the directory has one).
+- [ ] If a method was renamed or removed, all `AGENTS.md` references are updated.
+- [ ] Docstrings (if present) are accurate — not copy-pasted from a different method.
+- [ ] `conf/` changes are reflected in the test class's `AGENTS.md` parameter table (if one exists).
+
+---
+
+### Step 8 — Produce Review Output
+
+Structure comments by severity:
+
+| Level | When to use |
+|-------|-------------|
+| **BLOCKER** | Bug, security issue, data loss, or broken test — must fix before merge |
+| **MAJOR** | Violates TAF pattern, missing cleanup, wrong base class — fix strongly recommended |
+| **MINOR** | Style, naming, readability — fix preferred but not blocking |
+| **NIT** | Cosmetic (whitespace, comment wording) — optional |
+
+**Comment format:**
+
+```
+[BLOCKER] pytests/storage/fusion/fusion_base.py:142
+tearDown does not call super().tearDown(). Cluster cleanup will be skipped on failure.
+Fix: add `super().tearDown()` as the last line.
+
+[MAJOR] conf/fusion/fusion_sanity.conf:17
+Test entry references `fusion_sanity.FusionSanity.test_foo` but that method does not
+exist in the class. Will silently skip at runtime.
+Fix: correct method name to `test_foo_bar` or remove the entry.
+
+[MINOR] pytests/storage/fusion/fusion_sync.py:88
+Variable `bucketList` should be `bucket_list` (PEP8 snake_case).
+
+[NIT] pytests/storage/fusion/fusion_sync.py:92
+Comment "# loop over buckets" restates the code. Remove.
+```
+
+End with a one-line summary:
+```
+Overall: N blockers, N major, N minor, N nits. [Ready to merge / Needs fixes before merge.]
+```
+
+---
+
+## Quick Reference Checklist
+
+```
+Scope
+  [ ] Changed files identified and grouped by layer
+  [ ] Relevant AGENTS.md files read before .py files
+
+Correctness
+  [ ] Logic matches intent
+  [ ] Edge cases handled
+  [ ] No silent exception swallowing
+
+TAF Patterns
+  [ ] Naming conventions followed
+  [ ] Correct base class
+  [ ] Parameters via TestInputSingleton
+  [ ] tearDown cleans up resources
+  [ ] No hard-coded credentials or IPs
+  [ ] Submodule boundaries respected
+
+Configuration
+  [ ] Every new test_* method in pytests/ has a .conf entry
+  [ ] .conf entries match actual test methods
+  [ ] No duplicate entries
+
+Security
+  [ ] No secrets committed
+  [ ] No shell injection vectors
+
+Quality
+  [ ] No dead code or stale comments
+  [ ] No leftover debug prints (print, pdb, breakpoint)
+  [ ] No trailing whitespace
+  [ ] No duplicated utility logic
+  [ ] Imports clean (no unused, no circular dependencies)
+
+Documentation
+  [ ] AGENTS.md updated if public API changed
+```