Review

---
name: review
description: >-
  Review checklist for reviewer agents on chipping-orchestrator PRs. Use when
  evaluating a developer-produced branch before approval or change-requests.
---

# Reviewer skill — chipping-orchestrator

## CI / lint

Run `env -u VIRTUAL_ENV -u CONDA_PREFIX poetry sync` in a fresh review worktree before checks; `poetry run` does not
install or refresh dependencies. Include `--with docs` or `--with dashboard` when the change needs those optional
dependencies. Clear both markers for every Poetry command so an activated environment cannot override this `.venv/`.

Reject (or request fixes) if any of these are red:

- `.venv/bin/python -m ruff check orchestrator tests .github/scripts/docs_site.py`. Common offenders to look for
  explicitly:
  - **F401** — unused import on a package initializer. If the import is intended as a re-export, it must
    be aliased `from X import Y as Y` or listed in that initializer's `__all__`. A bare import will not
    survive ruff.
  - **F541** — f-strings without placeholders, typically in newly-added test files.
  - **F841** — unused local in tests.
  - **E402** — import after non-import code.
- `.venv/bin/python -m flake8 orchestrator tests .github/scripts/docs_site.py --select=WPS`. All WPS naming,
  complexity, consistency, bug-prevention, refactoring, and OOP findings are rejection criteria.
- `git diff --check origin/main...HEAD` — trailing whitespace and blank lines at EOF. Check it even
  if everything else looks clean.
- The full `.venv/bin/python -m pytest tests` passes on the reviewed commit — run it, or use current verification
  evidence the orchestrator handed you for exactly this subject (see [Verification evidence](#verification-evidence)).
  Reject "known failure" hand-waves; if the PR claims a baseline failure, the report must include a reproduction on
  `origin/main` at the branch point. Otherwise the developer must fix it.
- Every source file the PR adds (`*.py`, `*.sh`, `pyproject.toml`) places the `# Copyright 2026 Geser Dugarov` /
  `# SPDX-License-Identifier: Apache-2.0` header pair immediately after any shebang, or at the beginning of the file
  when there is no shebang.

## Verification evidence

The orchestrator hands you the workflow verification evidence current for exactly the head, requirements, and report
you are reviewing, when there is any, and publishes the evidence you declare on the pull request itself.

- End your final message with the verification declaration the prompt teaches, above the `VERDICT:` line: the
  `VERIFICATION: RUN <head>` block listing every command you ran with the exit status it returned, or
  `VERIFICATION: REUSED sha256:<revision>` naming the evidence you were handed. List only commands you actually ran,
  quote output as plain lines, and never infer a count or a result. An approval is acted on only when that evidence
  passed and includes every configured `VERIFY_COMMANDS` command, written exactly as the prompt lists it.
- Do not request changes merely so a developer or a human copies a commit SHA, a test count, or a command into the PR
  description or the report, while valid evidence covers the current tree and verification context. The orchestrator
  publishes that evidence with its provenance; a report describing an earlier run truthfully is not inaccurate.
- These remain actionable: a check that failed, verification the change needs and nobody ran, a report claiming
  something that did not happen or a result it did not observe, and evidence about another commit, report, or
  requirements revision than the one under review.

## Behavior preservation

For any refactor:

- Workflow labels, pinned-state JSON keys, comment marker text, watermark fields, and event-emission
  shape must match `main` exactly. Issues already in flight depend on these — a rename is a migration,
  not a refactor.
- Spot-check that moved code still routes through the same auth / fetch / push / retry helpers. A
  refactor is not allowed to silently change side effects.
- Squash-on-approval, the in_review HITL ready-ping gates (mergeable + approved + no standing
  CHANGES_REQUESTED), retry budgets, and stale-session detection are easy to break by accident during
  a move; verify their call paths survive intact.

## Workflow owners and stage modules

Most package initializers are markers. The deliberate public APIs retain their explicit `__all__` surfaces and
exact-path WPS410/WPS412 exclusions. `orchestrator.workflow` publishes the two label vocabularies, the transition
guard, its predicate and exception, and the lazy per-repo `tick` entry point. Labels live on `workflow/state.py`,
label parsing on `workflow/label_reading.py`, the graph on `workflow/transitions.py`, and write guards on
`workflow/transition_guard.py`.

- The workflow initializer imports no engine or stage. Its `tick` shim resolves `workflow/engine/tick.py` inside
  the call, so the GitHub and git layers can import labels and guards without an initialization cycle.
  `tests/workflow/test_imports.py` checks that direction, and `tests/repository/test_package_exports.py` holds the
  declared public surfaces and marker initializers.
- Settings reloads and patches target `orchestrator.config`, the same module object all callers retain.
  `config.RepoSpec` is the public alias of `config.models.RepoSpec`; callers that need only the type may import its
  defining owner. Token resolution is defined on `config.credentials`.
- Stage modules import the owner they borrow from at module scope and call through that alias; flag any
  reintroduced call-time hop through the package initializer.
- Test patches target the module the call site names. Flag a test that patches anything else — including
  the workflow package — since a mock left there intercepts nothing.
- Stage-private helpers stay in the stage package that owns them. Shared helpers are read from their defining
  owner; copying or re-exporting one creates a second patch target that can drift from the running call.
- Workflow owners declare `log = logging.getLogger("orchestrator.workflow")` with the channel spelled literally;
  `workflow/transition_guard.py` owns `orchestrator.state_machine`. Operator filters select on those names,
  and `tests/workflow/test_imports.py` checks them.

## Test economy and assertion quality

- Identify newly added tests that duplicate existing tests or each other; request merging into
  `pytest.mark.parametrize` cases or a small named loop when the only difference is fixture values or
  branch selection.
- Flag a class whose methods share one `setUp` scenario and differ only in which outcome they inspect.
  Ask for one test that runs the scenario once and asserts those outcomes together, keeping every
  assertion and its diagnostic message. Distinct scenarios stay apart: different failure inputs, crash
  windows, security boundaries, and side effects each earn their own run.
- Flag a test that patches a helper in the package under test and asserts `call_args`, object identity,
  or the order two same-package helpers ran in. Ask for the outcome instead — the workflow label,
  `pinned_data`, comments, and PR read back off the in-memory GitHub client for a stage handler, and
  the panel order, the selected filters and timezone, and the displayed data read back off the recording
  page doubles for a dashboard section. A seam patch is fine where the collaborator is deliberately
  stubbed and its effect cannot otherwise be observed — a network or database read, an agent run, a
  renderer reaching for an optional dependency — as long as it is patched on the defining owner.
- Do not extend that to counts and ordering that are themselves contracts — forbidden pushes, duplicate
  children or PRs, repeated charging, extra API reads or pinned-state writes, timeout cleanup,
  write-before-publish ordering — or to literal label, pinned-state key, comment marker, watermark, and
  event payload checks. Reject a PR that trades one of those for a looser outcome check.
- Reject a test-economy pass that drops a package's `test_imports.py` guards, or that probes fewer owners
  or environments than before. One fresh process per owner and per environment is the floor.
- Verify each added regression test fails before the fix and passes afterward.
- Require tests for changed contracts to assert those contracts directly.
- Allow tests that document and protect existing behavior, including coverage added before refactoring.
- For resource-usage fixes (over-fetching, redundant API calls, retained state), reject tests that
  only assert the final result; require at least one assertion at the helper/producer level.
- Prefer fewer tests with clear distinct coverage over many narrowly overlapping regression tests.
- Check placement: tests mirror the runtime layout, so a module under `orchestrator/<package>/` is covered by
  `tests/<package>/` and stage handlers by `tests/workflow/stages/<stage>/`. Flag a new omnibus module added beside
  an existing per-behavior split, a test parked away from the owner it exercises, and a stage reaching into a
  sibling stage's `*_test_support.py` for fixtures.

## Documentation drift

After a PR moves, renames, or deletes a symbol or module — a handler, helper, constant, or whole owner — grep the
PR for stale pointers to that name, inventory entries and docstrings included, and request fixes in:

- `docs/architecture.md` and the focused pages under `docs/architecture/` — the module-by-module inventory
  lives here and nowhere else, except `docs/architecture/observability-modules.md`, which maps
  `observability/` and `apps/` at the package boundary: flag a per-module entry grown back into that page
  rather than asking for one
- `docs/state-machine.md` and the focused pages under `docs/state-machine/`
- `docs/workflow.md` and the focused pages under `docs/workflow/`
- module docstrings at the top of the owners the symbol moved between, was renamed in, or was deleted from, and of
  the package initializers above them that describe where a name answers

`AGENTS.md` (and its `CLAUDE.md` symlink) is deliberately off that list, and the inverse is what to flag: it is
loaded into every agent session and carries no module, owner, or test inventory. Reject a PR that answers a routine
move, rename, or deletion of a symbol or module by editing it, or that grows an inventory back into it. It changes
only when repository-wide agent instructions, safety rules, or documentation routing change.

Treat blanket statements about what a package publishes — "every helper is re-exported", "the hub answers for
these names" — with suspicion; verify literally against the code, since an attribute that no longer exists raises
`AttributeError` rather than reading as stale prose.

## Comment hygiene

- Flag diff-relative comments — "previously", "the old retry cap", "instead of a dict", "now uses" —
  in code and test docstrings alike. A comment must read correctly to someone who never saw the
  change; the before/after story belongs in the commit message or PR description.
- Flag comments that paraphrase an already-readable line or the assert below them instead of stating
  a why (invariant, non-local consumer, prevented failure). Ask for the reason or for deletion. Do
  not flag plain-language summaries above genuinely dense code (tricky offset math, multi-step
  comprehension chains) — a comment that is faster to understand than the code it heads earns its
  place.

## `plans/` references

- `plans/` holds human working notes, not spec. Flag any code, comment, docstring, or test that cites
  a `plans/` document — or a numbered "Proposal N" from one — as authoritative; the change must stand
  on its own once that note is revised or deleted. Ask for the reference to be reworded to describe the
  behavior directly.
- A developer should not edit or remove files under `plans/` unless the issue explicitly asked. Flag
  unrequested `plans/` changes.

## Commit hygiene

- Conventional Commits: `<type>: <subject>` only. Reject any commit with a body, a `Co-Authored-By`
  trailer, or a non-imperative subject. Type must be one of `feat`, `fix`, `chore`, `docs`,
  `refactor`, `test`.
- Reject a developer commit whose subject ends in a numeric reference — ` (#N)` — the tracked issue's own number
  most of all. Publication references belong to the orchestrator, which appends the pull request's own reference when
  it publishes; a developer-written suffix lands a subject naming the issue and the pull request both. Read the
  subjects on the branch, not only the PR title, since approval planning can reuse a developer subject verbatim.

## Out of scope — push back

- Dependencies outside the issue's stated scope.
- Reformatting of files outside the change's blast radius.
- Abstractions or generality added for hypothetical future features. The issue's stated scope is the source of truth.