code_review.md

Code Review Guide

Use this review guide for benchmark, planner, training, and documentation changes that can affect published claims, reproducibility, or agent workflows.

Review Priorities

Check these in order:

1. Correctness of user-visible behavior and public contracts.
2. Benchmark credibility and provenance.
3. Proof that the new or changed behavior actually works in this repository.
4. Reproducibility of commands, configs, and artifacts.
5. Scope discipline: no hidden workflow churn or speculative infrastructure.
6. Downstream propagation completeness for evidence-producing PRs: parent issue, claim map or benchmark report, leaderboard or artifact catalog, registry, context index, and follow-up issue updates are either done or explicitly deferred.
7. Test and documentation coverage proportional to the risk of the change.

Reject changes that only add code or docs without task-appropriate proof.

Post-Merge Review-Thread Sweep

Merge does not mean "all review findings adjudicated." Inline review threads left unresolved on a merged PR are easily lost, and some are later fixed by successor PRs without ever being linked. After merge, run a short sweep so the thread ledger reflects reality:

1. Re-query unresolved review threads on the merged PR (gh api graphql on reviewThreads, filtering isResolved == false).
2. For each unresolved thread, record its discussion URL and one explicit disposition: covered with a fixing PR/commit, superseded with the replacement contract, decision_required with the concrete choice, or follow_up_required with a dedicated issue. Do not resolve the last two categories as if they were fixed.
3. Resolve or reply to covered and superseded threads when permissions allow. Keep the durable ledger truthful even when GitHub thread state cannot be changed in the current lane.
4. Record disposition counts in the merge note or linked issue so the sweep reconciles to the queried unresolved-thread count. The issue #5006 residual ledger is a compact worked example.

Treat substantive threads that need real work (for example a fail-closed contract gap or an end-to-end assertion gap) as follow-up issues, not as items to resolve with a link. This sweep is bookkeeping: it does not change benchmark, metric, or schema semantics.

Exact Merge-Commit CI Readback

After a guarded merge, the PR is terminal before its merge-commit checks necessarily finish. Use the watcher’s explicit commit mode to keep verification bound to the merge SHA:

uv run python scripts/dev/watch_pr_ci_status.py <pr-number> --post-merge-commit-sha <merge-sha> --json

This mode polls the exact commit check-runs and fails closed on a malformed or mismatched response. It proves workflow state only; it is not benchmark, research, release, or provenance evidence. Keep the merge receipt and exact commit SHA as the authoritative merge record. If a merge request returns a transport or server error before the PR becomes terminal, preserve the receipt's post_error_reconciliation evidence as unconfirmed; the current-main ref is diagnostic only and does not establish that the merge succeeded or authorize a retry.

Intended Design Alignment

Before treating CI or targeted tests as sufficient, compare the PR against the linked issue, design note, PR body, changed behavior, tests, docs, and stated claims.

Reviewers should explicitly classify the result:

• aligned: implementation behavior, tests, docs, and claims satisfy the intended design or issue contract.
• intentionally narrowed: the PR solves a useful subset, names the narrowed scope, and links follow-up issues for real deferred work.
• blocked: required contract, public-claim, benchmark, schema, metric, or runtime-safety work is missing and must be fixed before merge.
• handoff-only: remaining information is transient state, CI waiting, cleanup context, or local reviewer notes with no durable action.

Create a follow-up issue only for deferred work that is actionable outside the current PR. A good follow-up issue names:

• the residual risk or deferred behavior,
• why it should not block the current PR,
• the acceptance condition or stop rule,
• the expected validation or proof tier,
• links back to the PR, issue contract, design note, or evidence that revealed it.

Do not use generic backlog notes as substitutes for blockers or for follow-up issues with a clear acceptance condition.

Domain-Aware Approval Gate

Passing CI, automated review, and implementation-integrity checks is not sufficient for PRs that change evidence classification, experimental comparison methodology, figure eligibility, benchmark interpretation, or paper-facing claim surfaces. Those PRs need an explicit Domain-Aware Approval section in the PR body before they can be treated as merge-ready.

The gate is intentionally narrow. Ordinary implementation, docs, and test-only PRs can mark the domain approval requirement as not applicable when they do not alter evidence validity, comparison methodology, figure eligibility, or claim boundaries.

For PRs where the gate applies, reviewers should verify that the PR body names:

• target claim or hypothesis;
• comparator and split/evidence-validity policy;
• fallback/degraded exclusions;
• claim boundary and evidence tier;
• whether the change proves implementation integrity only, or also supports experimental validity.

PR #3276 is the motivating comparison-methodology example: automated review accepted the linked issue framing even though the comparison remained circular and lacked the held-out scenario-family design required by the issue. PR #3273 is the motivating evidence-classification example: machine-readable classifications and conservative prose diverged. Future PRs in those categories should be held until a domain-aware approval note is present, or should state a blocker instead of being presented as merge-ready.

Benchmark-Credibility Review

For benchmark-facing changes, explicitly verify:

Evaluation semantics

• Does the change alter success, collision, timeout, or metric semantics?
• Are fallback, skip, and fail-fast paths still explicit and testable?
• Are evidence statuses (diagnostic-only, smoke evidence, nominal benchmark evidence, paper-grade) still accurate?
• Does any report wording overstate what the benchmark actually measures?
• For mixed, partial, or fallback-tainted evidence, does the report start with claim boundary, evidence status, fallback/degraded exclusions, major caveats, and uncertainty before ranking or success language?

Observation normalization

• Are observation keys, bounds, clipping, and dtype contracts preserved or versioned?
• If a learned policy is involved, does the runtime observation contract still match the training contract?
• Are adapter transformations documented when planner inputs differ from env-native observations?

Scenario distributions

• Does the scenario set, seed policy, or map pool change what is being compared?
• Are scenario family changes reflected in configs, docs, and interpretation guidance?
• Does any new default silently bias benchmark outputs toward a subset of scenarios?

Reproducibility

• Is there a committed config or canonical command for the new behavior?
• Are generated outputs still rooted under output/?
• Do docs and PR text identify the exact command path needed to reproduce the result?
• If results depend on optional extras, hardware, or third-party assets, is that dependency explicit?

Upstream provenance

• For vendored, wrapped, or adapter-backed planners: is the upstream source still identifiable?
• Are licenses, checkpoints, model origins, and wrapper boundaries documented?
• Does the change preserve the claim that a result is original-code-backed rather than a local reimplementation?

Planner Integration Review

For planner additions or modifications, review:

• input contract: required observation/state fields are explicit,
• output contract: action space and kinematics adaptation are explicit,
• fallback policy: missing dependencies or artifacts fail clearly,
• provenance: upstream repo/model references remain auditable,
• proof: benchmark or targeted runtime evidence shows the planner actually runs in this repository,
• benchmark readiness: paper-facing vs experimental status is still correctly labeled.

If a planner is added but never executed in the local benchmark stack, treat the change as incomplete.

Planner or benchmark adapter PRs must cover every changed admission/fallback path: either add a direct regression assertion for each affected admission, provenance, quality-gate, or fallback field, or state precisely in the PR that the field is validated by preflight integration tests (rather than by a typed-constructor regression). Claiming direct regression coverage that the PR does not add is a review-blocking overclaim (issue #7610).

Native Execution Criterion For The #5579 Canary

Reviewers keep re-deriving an unsatisfiable probe on MPC canary work: treating planner_kinematics.execution_mode == "native" as if it were the #5579 "native execution" requirement. It is not, and the conflation has already cost multiple full quarantine / repair-exhaustion cycles (PR #6716 on 2026-08-07 and 2026-08-08). This section records the ruling so the same probe stops being re-derived. Ruling of record: issue #6828 (Option A, 2026-08-08); frozen criterion it interprets: issue #5579.

Two distinct concepts are being conflated. Keep them separate:

• execution_mode is a command-space concept. It is a planner_kinematics registry field that records which command space the planner emits into: native robot commands (native) versus adapter-projected commands (adapter) versus a mix (mixed). It is declared per algorithm in the registry (_KINEMATICS_PROFILE_BY_CANONICAL in robot_sf/benchmark/algorithm_metadata.py). It does not record whether a solver ran.
• Solver-execution is a runtime concept. It records whether the MPC solver actually ran and produced the commands. This is what the #5579 canary "native execution" clause guards.

The canonical prediction_mpc planner is registry-declared adapter-only: supports_native_commands: False, default_execution_mode: "adapter" (robot_sf/benchmark/algorithm_metadata.py, the prediction_mpc entry of _KINEMATICS_PROFILE_BY_CANONICAL). A gate bound to execution_mode == "native" is therefore unsatisfiable by construction for this planner. Demanding it on a PR is probing a defect that does not exist.

Ruling (Option A, issue #6828)

For a registry-declared adapter-only algorithm, the #5579 "native execution" requirement is satisfied by a fail-closed canary.solver_execution contract, not by planner_kinematics.execution_mode == "native". The fail-closed contract is what proves the solver actually ran. Concretely, the contract comprises:

• the solver token ran and produced commands (a non-degenerate solver update),
• the declared adapter execution shape (SOLVER_EXECUTION_IDENTITY_FIELDS),
• a registry cross-check that the declared adapter matches the runtime planner registry,
• the six fail-closed flags (SOLVER_EXECUTION_REQUIRED_FLAGS: require_valid_provenance, require_finite_commands, require_solver_update, require_control_update, forbid_solver_failure, forbid_fallback).

These are defined in robot_sf/benchmark/mpc_tuning_sensitivity.py and exercised by scripts/benchmark/run_mpc_tuning_sensitivity_issue_5579.py.

Boundary Preserved (Do Not Weaken)

This ruling changes only how "native execution" is satisfied for adapter-only algorithms. It does not relax the underlying requirement:

• For planners the registry declares supports_native_commands: True, the native command path is still required. Option A does not excuse a native-capable planner from its native path.
• Fallback or degraded execution never satisfies this criterion in either case (native-command or adapter-only). A run that fell back, skipped the solver, or executed in a degraded mode is a failure of the criterion, not a pass.

Reviewer Probe Guidance

When reviewing MPC canary work under #5579, do not raise execution_mode == "native" as a blocker for a registry-declared adapter-only algorithm such as prediction_mpc or learned_prediction_mpc. The benchmark-facing questions are instead: did the fail-closed canary.solver_execution contract run green (solver token ran, declared adapter shape matched the registry, all six fail-closed flags held), and was no fallback/degraded execution counted as success? If yes, the "native execution" clause is satisfied for an adapter-only algorithm. If a review still reads the frozen #5579 body text alone, use that coordinator-side issue-body amendment, this section, and issue #6828 together as the durable reading of the adapter-only interpretation.

Proof Requirements By Change Type

• new planner or planner integration: run a benchmark, policy-analysis path, or equivalent executable check that proves integration works here.
• metric update: provide targeted assertions, fixtures, or sample outputs that prove the new metric behavior.
• new skill: verify the referenced files, commands, and discoverability path are correct for this repository.
• new test: show that it protects a real contract or regression and is not only syntactic coverage.
• docs-only guidance: verify every referenced path and command, and ensure the guidance matches the current repo workflow.
• new research, benchmark, metric, or paper-facing analysis tool: require one representative use on durable/versioned input, or a linked follow-up issue that names the decision, claim boundary, benchmark report, registry, context note, or synthesis surface the tool will update. Local-only output/ files are not durable proof unless promoted or represented by a tracked manifest, registry entry, context note, or external artifact pointer. Small support helpers with no research-interpretation role may be marked NA with that reason.

Evidence Artifact Fallback Review

Use this checklist for evidence-producing PRs when automated AI review is rate-limited, unavailable, or path-filtered in a way that may skip small durable evidence files such as CSV tables under docs/context/evidence/.

• Confirm every linked issue, PR, context note, config, script, and evidence path resolves from a fresh checkout.
• Confirm copied evidence files are intentionally small and reviewable. Large raw logs, videos, model caches, coverage HTML, and raw episode JSONL should stay out of git unless a narrow fixture reason is stated.
• For CSV evidence, inspect the header and at least one representative row. Verify column names, units, seed/scenario/planner identifiers, and status fields match the surrounding Markdown or JSON summary.
• Check that parent issue conclusions, PR text, and context-note wording agree on result classification: benchmark evidence, smoke evidence, diagnostic-only, blocked, or proposal.
• Check claim-boundary language explicitly separates observed evidence from hypotheses, future work, fallback/degraded execution, and paper-facing claims.
• Check fallback/degraded rows are introduced as caveats or exclusions before any comparative ranking, aggregate success language, or recommendation.
• Check generated evidence points back to a reproducible command, commit, config or scenario matrix, seed policy, and durable artifact/provenance decision.
• If an evidence CSV or other small table is path-filtered out of automated review, state in the PR review or merge note that this manual fallback checklist covered it.

Documentation Review

Docs changes should be rejected if they:

• introduce benchmark claims without a reproducible command path,
• blur the line between observed evidence and future intent,
• describe experimental planners as baseline-ready,
• omit caveats around optional dependencies or upstream source limits.
• bury mixed-evidence claim boundaries or sub-95% confidence caveats after result interpretation.

Prefer docs that point to canonical config files, scripts, and issue execution notes instead of freehand operational prose.

Tests And Validation

Apply Principle XIII from .specify/memory/constitution.md:

• fix high-signal tests immediately,
• challenge flaky or low-value tests before investing in them,
• add tests when public contracts or benchmark semantics change,
• require proof that new tests or fixes meaningfully validate the intended contract,
• keep validation commands in PR text explicit and reproducible.

For shared-helper consolidations, require a per-call-site contract table and tests for every applicable behavior: return schema, missing and malformed input error/exit behavior, standalone import footprint, read strategy, path:line context, and output ordering. For serialization, subprocess, GPU-isolation, artifact-promotion, and CLI-handoff changes, require at least one one-real-path test that invokes production's serializer and dispatch path without manually pre-transforming the fixture.

See docs/context/issue_1436_reproducibility_flaky_acceptance.md for the canonical classification of deterministic, environment-class, and stochastic failures, and the explicit rerun boundary.

Recommended validation gate for broad changes:

BASE_REF=origin/main scripts/dev/pr_ready_check.sh

For docs-only or context-stack changes, still verify the modified markdown paths and any repo-local skill references.