review

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/.

Follow the logging and exit-status bullets when running tests to retain complete logs and preserve each test command's exit status. Run configured VERIFY_COMMANDS commands exactly as the prompt lists them, without adding flags or arguments. Treat added log redirection and exit-status capture as shell plumbing around each command; keep any redirection or status handling already present in a configured command.

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

• .venv/bin/python -m ruff check orchestrator tests fuzz .github/scripts. 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 fuzz .github/scripts --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). 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 its own exit status, or VERIFICATION: REUSED sha256:<revision> naming the evidence you were handed. List only commands you actually ran, declaring each command itself without the logging and status-capture plumbing you wrapped around it. 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.

Failure diagnostics

Give every failure a change request rests on an entry of its own under the numbered finding it supports, in the layout the prompt teaches, opening on the failure's name with each label on a line of its own:

Failure: <the exact test ID, a descriptive scenario name, or the check that failed>
Error: <the relevant error or assertion, as the output showed it>
Expected: <what should have happened>
Observed: <what happened instead>
Reproduce: <the command, or a self-contained snippet and the command that runs it>
Unavailable: <each diagnostic you could not obtain, and why>

• A failing test is named by its exact test ID as the runner reported it, parameters included (tests/pkg/test_mod.py::TestCase::test_name[case]), and reproduced by a command that runs that test alone.
• An ad hoc probe is named by a descriptive scenario name and reproduced by a self-contained snippet in a code block — every import and setup step it needs — with the command that runs it. A command reading unnamed code from stdin, such as .venv/bin/python -, reproduces nothing by itself. Distinct scenarios are distinct entries, each with its own name, error, and reproduction, even when one command ran them all.
• A failure no test ID names — a collection or import error before any test ran, a linter finding, a crashed command — is named by the check that failed and reported with its actual error. Never invent a test ID.
• Write Expected and Observed where the error alone does not explain the failure.
• Write Unavailable only where a diagnostic is missing, saying which and why, and still report the failure rather than leaving it out or guessing what you could not obtain.
• Entries and snippets belong to the findings, never inside the verification declaration, and none of their lines opens on VERIFICATION:, COMMAND:, or EXIT:. The declaration still lists each command exactly as you ran it, with its own exit status: a stdin probe stays .venv/bin/python -, once per run.

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 deliberate format change under C-03, 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.

One writer per issue

Each issue has one active orchestrator writer: dispatch and the per-tick base refresh take its writer claim before reading it and hold it through their last write (design constraints).

• Do not request support for a second orchestrator writer changing the issue's pinned comment or labels during a claimed operation -- adopting a successor it settled there, reconciling records it moved and put back, or wrapping every reading of the comment to catch it -- and flag a PR that adds such machinery.
• Do request handling for what still happens under the claim: a process lost between two writes, a write refused or whose answer was lost, a comment with no room left, and changes from outside the orchestrator -- humans, labels, issue text, pull requests, remote branches, configuration, and checkouts.
• A successor or replacement another orchestrator operation wrote between two separately claimed operations is real; the reading taken once the claim is held is what sees it, so do not dismiss that scenario as an excluded writer.

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.