review

# PR Review (pygraphistry) ## Invocation ```text /review [<PR-number-or-branch>] [mode=findings|pr-comments|both] [fixes=deferred|inline] ``` Defaults: - Target: current branch PR (`gh pr view --json number,headRefName,baseRefName,url`) - `mode=findings` - `fixes=deferred` `mode`: - `findings`: local artifacts only under `plans/<task>/` - `pr-comments`: draft locally, post only after explicit user confirmation in the same session - `both`: run findings then comment flow `fixes`: - `deferred`: read-only review - `inline`: after each converged wave, apply confirmed `BLOCKER`/`IMPORTANT` fixes in separate commits ## Runtime Assumptions - Run from repo root. - `gh` is authenticated (`gh auth status`). - Local branch reflects PR head (`origin/<base>...HEAD` matches intended review scope). - `plans/` is local working memory and normally gitignored. ## Plan-First Requirement Always use the plan skill flow: 1. If `plans/<task>/plan.md` exists, reuse it and append a review section. 2. Else reload `.agents/skills/plan/SKILL.md` and create `plans/<task>/plan.md`. 3. Record PR metadata, `mode`, `fixes`, and timestamp. 4. Reload plan before every step; update plan immediately after every step. `<task>`: prefer `review-pr-<N>` or `<branch>-review`. ## Phase 0: Resolve Scope + Stack Context 1. Resolve PR context (number/title/url/head/base). 2. Record stack context: ```bash gh pr view <PR> --json baseRefName,headRefName,title,body gh pr list --base <headRefName> ``` 3. If stacked, explicitly mark out-of-scope upstream/downstream work in `plan.md`. 4. Set diff range reference: `origin/<base>...HEAD`. ## Phase 1: Research Criteria Before Findings Create `plans/<task>/research/` with: - `context.md` - `policies.md` - `credentials.md` - `canvas-<dimension>.md` (only for dimensions that apply) ### 1a) Collect context + changed files ```bash gh pr view <PR> --json number,title,headRefName,baseRefName,url,body git fetch origin <baseRefName> <headRefName> git diff --name-only origin/<base>...HEAD git log --oneline origin/<base>..HEAD ``` Record linked issue/spec refs from PR body and summarize PR intent. ### 1b) Discover applicable repo rules Always inspect: - `AGENTS.md`, `DEVELOP.md`, `ARCHITECTURE.md`, `CONTRIBUTING.md`, `README.md`, `CHANGELOG.md` - `docs/source/**` relevant to changed areas - CI/workflow context under `.github/workflows/` when checks/publish behavior are touched - Tooling configs (`pyproject.toml`, `mypy.ini`, `pytest.ini`) and helper scripts in `bin/` Walk up from each changed file to repo root and include nearby `.md` guidance. ### 1c) Credentials gate (always, early) Run before wave analysis: ```bash git diff origin/<base>...HEAD -- '*.env*' 'custom.env*' 'docker-compose*.y*ml' '*.conf' '*.config.*' | \ grep -iE '(api_key|secret|token|password|bearer|authorization|aws_|azure_|openai_|anthropic_).{0,4}=' || \ echo "[clean] no obvious credential strings" git diff origin/<base>...HEAD | \ grep -oE '(sk-[A-Za-z0-9]{20,}|AKIA[0-9A-Z]{16}|Bearer\s+[A-Za-z0-9._-]{20,})' | head ``` If any likely secret is found, raise `BLOCKER` and stop until surfaced. ## Phase 2: Multi-Wave Review Loop Run waves until 2 consecutive waves show no significant advance. Severity: - `BLOCKER`: merge must not proceed - `IMPORTANT`: should fix before merge - `SUGGESTION`: non-blocking improvement Suggested folder layout: ```text plans/<task>/waves/wave-<N>/ <dimension>/findings-<file-slug>.md <dimension>/report.md adversarial/<finding-id>.md adversarial/report.md wave-report.md ``` ### Wave gate (start of each wave) 1. Reload `.agents/skills/plan/SKILL.md` 2. Reload `plans/<task>/plan.md` 3. Confirm current diff SHA and wave number 4. If prior wave had substantive findings, perform both: - targeted amplification pass on prior findings/fixes - clean-slate full pass on current diff ### Dimensions Apply only relevant dimensions per PR: - Spec conformance - Correctness - Testing - Security - Code quality - DRY / reuse - Concurrency / parallelism - Performance - Architecture - Operability - Repo conventions Guidance: - Keep dimensions independent (avoid blended "general review" prompts). - For file-heavy diffs, parallelize by `(dimension, file)` and aggregate. - Verify pre-existing patterns are not misreported as regressions: ```bash git show origin/<base>:<file> ``` ### Pygraphistry-specific review checks - Test coverage should mirror changed areas in `graphistry/tests/**`. - Prefer behavioral tests over implementation-detail assertions. - Treat broad exception catches (`except Exception`, bare `except`) as a bad dynamic/over-defensive typing pattern by default. Require narrowed, documented exception types at local helpers; allow broad catches only at explicit isolation boundaries where the PR explains why programmer errors must be contained. ### Encoding: names, tests, structure — not prose Meaning belongs in a **name**, a **test**, or the **structure**; a comment is the last resort. Delete from a diff: narration of the next block (extract a helper named for the rule), a why-this-fix or issue-number rationale (the pin's test name carries it; an issue ref may stay as a trailing tag), any perf/complexity/benchmark claim (measurement belongs in pyg-bench), and restatements of the signature. A keep must state a constraint that no name and no test can express — *defensible* is not the bar, and a doubtful keep deletes. Guard: `bin/ci/ci_comment_density_guard.py`. Typing, same rule: `Any` over a known domain gets the real alias; a new `# type: ignore` or `hygiene-ok` gets restructured (both are for grandfathered debt only); `cast()` to satisfy the checker becomes `isinstance` narrowing. Hygiene-guard "no growth" is not sufficient — a file the PR touches should go down. Run this skill on your own `<base>..HEAD` diff before pushing, and carry these rules in any brief you hand a subagent. Re-fetch review comments before calling a thread addressed: anchors are per-commit (`commit_id`, `original_line`), and your remediation commit gets reviewed too. ```bash git diff <base>..HEAD -- 'graphistry/**/*.py' ':!graphistry/tests/**' \ | grep -nE '^\+\s*#|^\+.*(\bAny\b|type: ignore|hygiene-ok|cast\()' | grep -v 'from typing import' ``` - Control-flow checks (runtime code, tests, and prompt-routing logic) must use structured signals (for example `code`, context keys like `field`/`value`, AST/symbol kinds, and stable symbol-binding metadata/files) and never **hardcoded** message-substring matching. - Run focused validation before escalating severity when feasible: ```bash python -m pytest -q [targeted_test] ./bin/ruff.sh [changed_path_or_pkg] ./bin/typecheck.sh [changed_path_or_pkg] ``` - For GPU-affecting PRs, require GPU-path validation evidence: - Local GPU path: `cd docker && ./test-gpu-local.sh [targeted_test_or_path]` - No local GPU available: run equivalent GPU validation on `dgx-spark` and record exact command + output artifact path in wave evidence. - For RAPIDS/cuDF changes, prefer dual-version validation (`RAPIDS_VERSION=25.02` and `26.02`) and include at least one amplified pass beyond early-stop defaults (for example, avoid relying only on `--maxfail=1` harness behavior when triaging regression surface). - When shared GPU pressure blocks full-matrix execution, require explicit evidence of the constrained condition (for example `nvidia-smi` + failing stack site), then run targeted amplified subsets and document exactly which tests were excluded and why. - If startup/runtime claims are made, verify entrypoints/scripts in `bin/` and workflow behavior. - For docs-only PRs, prioritize spec/documentation accuracy and navigability (toctree links, anchors, cross-refs). ### GFQL change boundaries, engines, engagement, perf (compute/**) - **Both sides of every boundary.** A fix carries a test that fails with the defect reinstated and passes with the fix; the case that must still decline or raise is pinned next to it. A differential with zero rows on both sides, or where another route served the query, tests nothing: assert the expected row count and the serving route. - **Sweep the siblings.** A change to one engine, lane or shape is checked on every sibling before it is landable: pandas, cuDF, polars, polars-gpu, and each specialization of the same shape (`chain_specializations/`, `gfql/lazy/engine/polars/chain_specializations/`, the indexed bindings kernel, the row pipeline). Report the siblings that were probed and what each did. - **Engagement is a pin, not a timing.** "The index/fast path is used" is proven by a `gfql_explain` assertion (`used_index`, `decision_code`, the seam name) marked `@pytest.mark.route_engaged(...)` so `bin/test-routes-off.sh` can replay the parity half with the route disabled. Parity stays an unmarked result pin. A wall-clock assertion in pygraphistry tests is a finding. - **Perf claims live in pyg-bench.** A number in a PR body or CHANGELOG needs a pyg-bench measurement with an A/A control beside the A/B, pinned in that repo's thresholds + contract test; pygraphistry carries results and data contracts only. Local-box numbers do not close a perf PR. - **Cypher surface.** A change reachable from a Cypher query (parser, DDL, row pipeline, WHERE/RETURN lowering) runs the tck subset that covers it; engine-parametrize the result pin. - **Release notes and docs.** Every user-visible change has a CHANGELOG.md entry under `[Development]` -- never inside a released `## [x.y.z - date]` section, which is history. Docs edits are minimal, plain English (ASD-STE100: short sentences, one meaning per word, no internal jargon or issue chatter), and a stale sentence is deleted rather than hedged. - **An identity claim is a measurement.** "Same answers", "value-identical", "cost only" needs an A/B of old against new over adversarial shapes -- nulls first, a failing value past any sample, all-null, empty, shorter than the sample, mixed element types, each engine dtype family -- and the claim states how many shapes were compared and how many diverged. A surviving divergence is disclosed as a correction in the CHANGELOG and pinned, never dropped from the count. - **Close the class, do not narrow the claim.** When a defect has siblings, prefer one chokepoint that fixes every producer over a per-site patch plus a narrowed sentence. Narrowing is the fallback, and it names the shapes left open. - **Never pin behavior that differs by environment.** A pin that passes locally and fails on another Python or dependency set is pinning the environment, not the contract. Pin the invariant both outcomes must satisfy (the error code and field, or the rows), and report the divergence. - **A comment run is one line.** `bin/lint.sh` counts a two-line comment as a finding, so say it in one line or put it in a name. #### Vectorization & engine compatibility (GFQL / row pipeline / compute) GFQL is vectorization-first and pure-functional. The row pipeline runs on pandas + cuDF; per-row Python loops are a pandas perf cliff and a cuDF break, and in-place mutation creates aliasing surprises. Audit edits in `graphistry/compute/**` (esp. `gfql/**`, `plotter/**`) for the patterns below. **Engine-polymorphic helpers** (use these instead of pandas-only APIs in compute paths): - `graphistry.Engine.df_cons(engine)`, `df_concat(engine)`, `df_to_engine(df, engine)`, `safe_merge`, `resolve_engine(arg, df)` - `graphistry.compute.dataframe_utils.template_df_cons(template_df, data)` - Type DataFrame params as `DataFrameT` (`graphistry.compute.typing`), not `pd.DataFrame`. **Vectorization** — flag (BLOCKER on hot rows / IMPORTANT elsewhere unless noted): | Pattern | Fix | |---|---| | `df.apply(fn, axis=1)`, `iterrows()`, `itertuples()` | Vectorized ops on Series | | `for x in df[col]` building a Series | `Series.map` / numpy / mask | | `sum(s)` / `max(s)` / etc. on a Series (SUGGESTION) | `s.sum()` / `s.max()` | | `df[col][i]` scalar access in a loop | Same vectorization fix; cuDF rejects this | **Mutation** — pygraphistry is pure-functional; flag IMPORTANT (BLOCKER if cell-wise in a loop): | Pattern | Pure alternative | |---|---| | `df[col] = v` / `df.col = v` | `df.assign(col=v)` | | `df.loc[mask, col] = v` | `df.assign(col=df[col].where(~mask, v))` | | `df.loc[i,c]=` / `df.iloc[]=` / `df.at[]=` in a loop | Build column vectorized + `df.assign(...)` | | `inplace=True` (drop/rename/sort/fillna/...) | Drop kwarg; assign return | | `del df[col]` | `df.drop(columns=[col])` | | `df.append(...)` (deprecated; not in cuDF) | `df_concat(engine)([df, other])` | | Mutating then returning the input df | Return a new df; no observable side effects | Vectorized mutation (`df.loc[mask, col] = v`) is still mutation — prefer `df.assign(...)`. **Batch the assign** (SUGGESTION; IMPORTANT on a hot row path): copy-then-mutate (`out = df.copy()` followed by one or more `out[col] = ...`) should be a single `df.assign(col_a=..., col_b=...)`. It is the functional phrasing the rest of the codebase uses, it drops the explicit copy (assign already returns a new frame), and one assign builds the result once instead of copying and then writing column by column. | Pattern | Fix | |---|---| | `out = df.copy()` + `out[c] = v` | `df.assign(c=v)` | | `out = df.copy()` + several `out[c] = v` | one `df.assign(c1=v1, c2=v2)` | | `df.copy().rename(columns=...)` | `df.rename(columns=...)` — rename already copies | | `out = df[mask].copy()` with no later mutation | `df[mask]` — masking already copies | **cuDF compatibility** — flag IMPORTANT unless noted: | Pattern | Fix |