Record review expectations for comments and test assertions

.coderabbit.yaml

@@ -107,6 +107,14 @@ reviews:
       instructions: |
         C++ tests (gtest). Focus on:
         - Numerical correctness validation (not just "runs without error")
+        - Assertions that pin the expected answer. EXPECT_NE(x, sentinel) and
+          "returns without error" pass for a solver producing the wrong number;
+          ask for EXPECT_NEAR against a computed optimum. A sentinel is right
+          only when the point is that a buffer must stay untouched.
+        - Complete output buffers, not just element [0], so an accessor that
+          fills a prefix and stops cannot pass
+        - Duplicate coverage. If a new test exercises a path the suite already
+          covers, ask what it adds that existing tests do not
         - Edge cases: empty, infeasible, unbounded, degenerate, singleton problems
         - Test isolation — no leaked GPU state or global mutation across tests
         - Flakiness: GPU timing races, uninitialized memory, non-deterministic order

.github/.coderabbit_review_guide.md

@@ -32,7 +32,8 @@ Any comment on them duplicates CI noise:
 
 - Bikeshed naming (unless the name is actively misleading, e.g., hides a GPU↔host boundary or units)
 - Splitting functions "for readability" without a concrete maintainability trigger
-- Comment density preferences
+- Comment density preferences — how *many* comments a change carries is taste.
+  Comment *content* is not: see "Comments" under C++ conventions.
 - Nits on lines the PR did not change
 
 ---
@@ -61,6 +62,13 @@ from the actual code and from `.clang-format`.
   Exceptions are the canonical mechanism — do not flag exception use.
 - **Formatting**: handled by `clang-format` (`BasedOnStyle: Google` with cuOpt
   overrides). Do not comment on formatting at all.
+- **Comments**: a comment states what the code does or why it is non-obvious.
+  It does not narrate the bug that motivated it, the alternatives considered,
+  or what a reviewer asked for — that belongs in the commit message or PR.
+  Flag narration in a comment; do not flag a missing comment on self-evident
+  code, and do not flag comment count. See also `skills/cuopt-developer/references/conventions.md`,
+  which additionally forbids volatile details (line numbers, commit hashes, PR
+  numbers) in comments.
 
 ### C++ — language-level practices we follow from Google C++
 
@@ -293,6 +301,12 @@ Tests reference these paths via the `RAPIDS_DATASET_ROOT_DIR` environment variab
 6. **API stability** — `cuopt_c.h` changes; Python `DeprecationWarning`; server endpoint versioning.
 7. **Security** (server paths only) — input validation, size limits, deserialization.
 8. **Ask, don't tell** — "Have you considered X?" not "You should do X."
+9. **A maintainer's decision settles the thread.** If a reviewer has already
+   answered a finding of yours on the same lines — accepting the behaviour,
+   choosing a different trade-off, or judging the case not worth handling — do
+   not re-raise it on the next pass. Treat it as resolved even where the
+   mechanism you described was correct, and say so briefly rather than
+   repeating the original argument.
 
 ---
 

skills/cuopt-developer/references/conventions.md

@@ -23,6 +23,23 @@ window_count)),  // NOSONAR: window_count declared before window_state_ (line 86
 
 This applies to all comment types: inline comments, block comments, suppression directives (`// NOSONAR`, `// NOLINT`, `# noqa`, `# type: ignore`), and doc comments.
 
+**Comment what the code does, not how it came to be.** A comment explaining the bug that motivated a check, the alternatives that were considered, or what a reviewer asked for is narration. It belongs in the commit message or the pull request, where it is dated and attributed. In the code it reads as noise to everyone who arrives later without that context.
+
+Add a comment where the code is genuinely non-obvious — an unusual invariant, a non-local dependency, a deliberate deviation. Self-evident code needs none.
+
+```cpp
+// ✅ GOOD — explains a non-local dependency the reader cannot see here
+// Empty here is valid for a problem with no constraints, so the solve is what is tested.
+if (solution->get_solution_host().empty()) { return CUOPT_INVALID_ARGUMENT; }
+
+// ❌ BAD — narrates the fix rather than the code
+// An empty vector means the solve produced no values, not that every value is zero.
+// Copying nothing and reporting success would leave the caller's buffer at whatever it
+// held and give them no way to tell the difference. Review pointed out that the earlier
+// version regressed the zero-constraint case, so this now tests the solve instead.
+if (solution_host.empty()) { return CUOPT_INVALID_ARGUMENT; }
+```
+
 ## C++ Naming
 
 | Element | Convention | Example |
@@ -158,3 +175,18 @@ Read existing code in `cpp/src/` for real examples of RMM allocation, stream-ord
    - Python pytest: `python/.../tests/`
 
 **Add at least one regression test for new behavior.**
+
+**Assert the expected answer, not that something happened.** `EXPECT_NE(x, sentinel)` and "returns without error" pass for a solver that produces the wrong number. Work out the correct result and assert it with `EXPECT_NEAR` or `EXPECT_EQ`. A sentinel is the right assertion only when the point is that a buffer must be left untouched.
+
+```cpp
+// ✅ GOOD — minimizing x over 0 <= x <= 5 has a known optimum
+EXPECT_NEAR(primal[0], 0.0, 1e-6);
+EXPECT_NEAR(reduced[0], 1.0, 1e-6);
+
+// ❌ BAD — passes for any value the solver happens to write
+EXPECT_NE(primal[0], sentinel);
+```
+
+**Assert every element of an output buffer**, not just the first. An accessor that fills a prefix and stops will pass a test that checks `buffer[0]` alone.
+
+**Check whether the case is already covered before adding a test.** Grep the existing suite first. A test added for symmetry — "the failing path is covered, so cover the working path too" — is worth nothing if the working path already has coverage, and it is one more thing to maintain. Being able to answer "what does this cover that existing tests do not?" is the bar.