| name | devops-bench-review |
| description | Use when the user asks for a CODE review of devops-bench changes — e.g. "review this PR", "review my changes", "review the working tree", "code-review this diff", "is this harness/deployer/metric change sound". Reviews a PR (number/URL) or the current working tree across seven code lenses — correctness, testability, maintainability, API hygiene, domain modeling, conventions, and security — and returns ranked, actionable findings with severity + file:line evidence + a concrete fix. Review-only: it analyzes statically and may run unit tests, ruff, and format checks, but it NEVER runs benchmark evals or provisions infra. For a NEW or CHANGED benchmark task (task.yaml + its stack), use the sibling `task-review` skill instead.
|
devops-bench code review
Review a GitHub PR or the current working tree as code, then return
ranked findings a maintainer would act on. Each finding is
severity (blocker / major / minor / nit) + file:line evidence + a concrete,
actionable fix, scoped to the change. Do not nitpick; do not invent findings to
fill a quota. If nothing survives, say so.
devops_bench/ is the canonical pipeline — ignore legacy pkg/ unless the
change is in it. For the layering, registries, and lifecycle, read
architecture and
glossary rather than reconstructing them.
Defer task-specific concerns — schema/metadata, spec parsing, outcome rubrics,
and the per-task parallel-safety of cloud resource names — to the
task-review skill. This skill reviews code.
Scope & guardrails — review only
Analyze and report. Do not execute the benchmark, and never provision infra.
- May run (only to validate the code under review): unit/integration tests for
the changed code (
uv run pytest), uv run ruff check ., and
uv run ruff format --check .. Report violations; do not reformat files as part
of the review.
- Must NOT run:
python -m devops_bench, the matrix scripts, any agent/judge
invocation, or tofu/gcloud/kind/kubectl apply/destroy. If judging a
change seems to require running it, report what static analysis shows and state
that an actual eval is out of scope.
If a lens needs a capability (sub-agent for an independent verifier pass, etc.),
express the need generically and consult
harness-capabilities; degrade to doing
it inline.
Gather the diff
- A PR (number/URL):
gh pr view <t> --json title,body,baseRefName,changedFiles
and gh pr diff <t>. Read enclosing code from this checkout if it matches the PR
branch, else git show <ref>:<path>.
- Working tree:
git diff @{upstream}...HEAD (or main...HEAD) plus
git diff HEAD for uncommitted work — review is often pre-commit. Treat the union
as scope.
The diff is the scope. For each touched function, also read the enclosing function:
a bug on an unchanged line of a touched function is in scope (the change re-exposes
or fails to fix it).
Lenses
Apply the lenses that fit the change. Most code wants Correctness, Testability, and
Conventions; library/registry surfaces add API hygiene and Domain modeling.
Correctness
Logic and edge cases: inverted/off-by-one conditions, null/empty/missing-key paths,
falsy-zero checks, missing await, swallowed exceptions, wrong-variable copy-paste,
set -euo pipefail gaps in bash. No hallucinated APIs — every called function,
attribute, registry key, env var, and CLI flag must exist (Grep the symbol; check
the registry decorator). For each deleted/replaced line, name the invariant it
enforced and confirm it is re-established elsewhere — a dropped guard or error path
is a finding. For each changed function, check callers/callees: does a new
precondition, changed return shape, or new exception break a call site?
Testability
New or changed logic should have tests that would actually fail on breakage —
not tautological (asserting the mock returned what the mock was told to return,
re-deriving the expected value with the code under test, or assert x == x). Check
edge coverage (empty, error, boundary), not just the happy path. Flag new
non-trivial logic with no test as a finding, and name the test that should
exist. Note when code is hard to test because a dependency is hard-wired rather
than injected.
Maintainability
Complexity and over-engineering: speculative config/flags/abstraction for a future
that isn't here, parameters no caller passes, premature generalization. Respect the
layering — core/ → {models, providers, deployers, agents, chaos, verification, metrics} → evalharness/. An inward import (e.g. core/ importing evalharness/,
or one sibling reaching into another's internals) is a finding; name the seam it
should cross instead. Prefer the smallest change that solves the actual problem.
API hygiene / design
Public surfaces should be clear and stable: an extension axis is added by
registering a class via the matching decorator (AGENTS, MODELS, PROVIDERS,
FAULTS, TRIGGERS, VERIFIERS, METRICS) — flag a change that edits the engine
to special-case a new variant instead of registering it. No leaky abstractions
(callers depending on internals, or a return type that exposes implementation).
Watch __all__ / signature changes that break the public contract without reason.
Domain modeling
Types should model the domain. The repo already has the right vocabulary — Task,
AgentResult, ClusterInfo, RunContext, RunEnv, MetricScore,
VerificationSpec. Flag primitive obsession where one of these (or a small new
dataclass) belongs: a bare dict/tuple/positional-string passed across a seam
that a typed object would make self-describing and validate-once. Flag stringly-typed
state that should be an enum, and parallel lists that should be one list of records.
Conventions
- Tooling:
uv for everything (uv run …, uv add … — not bare pip/python).
- Lint: ruff with
E, F, I, UP, B, SIM, line length 100. Run uv run ruff check ..
- Docstrings: Google style — purpose;
Args / Returns / Attributes;
Raises; concise, no implementation narration.
- Comments — over-commenting is a finding. Self-documenting code needs no
running commentary. Flag any comment that narrates what the code does
(
# loop over the items, # increment counter, a docstring-restating-the-body).
Keep a comment only when it explains a genuinely non-obvious edge case or
intent the code can't show (a 409-on-re-run workaround, a length-limit
rationale). When you flag one, say whether the fix is "delete it" or "rewrite it
to explain the why".
Security
Secrets and inputs: no committed credentials, keys, or tokens (Grep the diff for
obvious patterns); secrets read from env/secret-store, not hardcoded; user/agent/
task-supplied strings that reach a shell are validated or passed argv-style (no
shell=True interpolation); no path traversal from un-sanitized names. Flag a
secret echoed into logs.
Verify, then present
Dedup candidates pointing at the same mechanism. For each survivor, run an
independent verifier pass on non-obvious ones (a sub-agent if available, else
re-check yourself) and try to refute it by finding the guard/test/type that
already covers it. To corroborate, you may run uv run pytest and
uv run ruff check . — pre-existing failures on untouched code are not the
author's (note them as context, not findings). Drop anything refuted.
Present a readable review (not raw JSON):
- Overview — 1–2 sentences on what the change does.
- Findings, most-severe first, each as
severity — file:line — summary then a one-line failure/why and the concrete
fix, and how to verify (the test to add/run, the ruff rule, the call site to
check).
- Cleared — a short list of what you checked and found sound, so the author
knows the coverage.
- Systemic note (when applicable) — if several findings share a root cause,
recommend the seam-level fix once instead of per-site patches.
Scale effort to the ask. Never run the benchmark to produce a finding.
