fix(casework): exit non-zero on failed submissions, tighten the review nits

CodeRabbit review on #444:

- main() returned 0 even when every POST failed, which is the same hole the
  credential and write-guard aborts exist to close. Now returns 1 if any case
  errored. This diverges from the sibling enrichers, which always return 0;
  the divergence is documented in the README.
- README said "Two failures" then "All three" of the same list.
- submit_review built its headers by hand instead of using _headers's
  content_type parameter, as create_material already does.
- Cover the bare-list branch of reviews_for_slug; pagination is a project-wide
  DRF setting, so the untested branch could go stale silently.
- Build the render_report test row through _row -- the literal still carried
  the `title` key that was dropped.
- Record why --force is load-bearing in the guard-wiring test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: gaurav-karki

casework/README.md

@@ -506,15 +506,21 @@ the batch is the authority, and a PUBLISHED case is a legitimate review target.
 
 Two failures abort the run instead of being counted per case: a credential rejection
 (**401** an expired or invalid token, **403** a valid token without the Caseworker
-role) and the write-guard refusal (a remote host without `--allow-remote-writes`). All
-three fail identically on every remaining case, so counting them would bury one
-configuration mistake under several hundred warnings and still exit 0.
+role) and the write-guard refusal (a remote host without `--allow-remote-writes`). Both
+fail identically on every remaining case, so counting them would bury one configuration
+mistake under several hundred warnings.
 
 Everything else is per-case. A read or POST that fails on one slug costs that slug and
 the batch continues; in `--report` it becomes an `unreadable` row rather than losing the
 whole file. Recover a failed subset with `--slug <a> --slug <b> --force` — re-running
 the whole batch with `--force` re-grades every passing case too.
 
+**Exit code, and how it differs from the other stages.** `submit_reviews` returns **1**
+when any case failed, 0 otherwise. Every other stage in this package always returns 0.
+The divergence is deliberate: this one is meant to be chained after the enrichers in a
+script, and a batch where every POST 500'd is otherwise indistinguishable from a clean
+run. Skips and dry-run `would_submit` counts are not errors.
+
 ---
 
 ## End-to-end production runbook

casework/common/api.py

@@ -319,9 +319,8 @@ def submit_review(self, slug, timeout=60):
         """
         url = self.base_url + "/casework/reviews/submit/"
         body = json.dumps({"slug": slug}).encode("utf-8")
-        headers = dict(self._headers())
-        headers["Content-Type"] = "application/json"
-        with self._request("POST", url, data=body, headers=headers,
+        with self._request("POST", url, data=body,
+                           headers=self._headers("application/json"),
                            timeout=timeout) as r:
             return json.loads(r.read().decode())
 

casework/submit_reviews.py

@@ -376,7 +376,11 @@ def main(argv=None):
                    duration_s=time.monotonic() - started)
     print(f"\n=== submit reviews ({'DRY RUN' if args.dry_run else 'APPLIED'}) ===")
     print(f"  {format_counts(stats)}")
-    return 0
+    # Exit non-zero when any case failed. This DIVERGES from the sibling enrichers,
+    # which always return 0: a wrapper cannot otherwise tell a clean batch from one
+    # where every POST 500'd, and the abort paths above exist precisely to stop a
+    # configuration mistake exiting 0. Skipped and would-submit cases are not errors.
+    return 1 if stats["error"] else 0
 
 
 if __name__ == "__main__":

tests/casework/test_api.py

@@ -1077,6 +1077,15 @@ def fake_get(path, params=None, timeout=60):
     assert rows == [{"id": 1841, "status": "done"}]
 
 
+def test_reviews_for_slug_accepts_an_unpaginated_list(monkeypatch):
+    """Pagination is a project-wide DRF setting, not this view's promise -- turning
+    it off must not make the skip check see zero reviews and re-grade the corpus."""
+    api = CaseworkApi("http://127.0.0.1:48010", basic=("u", "p"))
+    monkeypatch.setattr(api, "get", lambda *a, **kw: [{"id": 1841, "status": "done"}])
+    rows = api.reviews_for_slug("case-078-cr-0038-ciaa-special-court-case-078-cr-9a")
+    assert rows == [{"id": 1841, "status": "done"}]
+
+
 def test_reviews_for_slug_on_a_never_reviewed_case_is_empty(monkeypatch):
     api = CaseworkApi("http://127.0.0.1:48010", basic=("u", "p"))
     monkeypatch.setattr(api, "get", lambda *a, **kw: {"count": 0, "results": []})

tests/casework/test_guard_wiring.py

@@ -196,6 +196,9 @@ def no_sockets(*a, **kw):
     batch.write_text(
         "slug\ncase-078-cr-0038-ciaa-special-court-case-078-cr-9a\n", encoding="utf-8")
 
+    # `--force` is load-bearing: it skips the pre-check GET. Reads are not
+    # write-guarded, so an unforced run would reach `no_sockets` first and fail with
+    # AssertionError before the POST guard ever fires.
     with pytest.raises(RuntimeError, match="refusing to write to non-loopback"):
         sr.main(["--batch-csv", str(batch), "--api-base-url", NON_LOOPBACK_BASE_URL,
                  "--api-token", "t", "--apply", "--force"])

tests/casework/test_submit_reviews.py

@@ -180,8 +180,8 @@ def test_summary_counts_dispositions_and_scores():
 
 
 def test_the_rendered_report_names_every_case_and_the_totals():
-    rows = [{"slug": SLUG_A, "review_id": 1841, "status": "done", "score": 84,
-             "disposition": "PASS", "duration": 92.4, "title": "", "error": ""}]
+    rows = [sr._row(SLUG_A, "done", review_id=1841, score=84,
+                    disposition="PASS", duration=92.4)]
     text = sr.render_report(rows, sr.summarize(rows),
                             base_url="http://127.0.0.1:48010/api",
                             run_id="testrun", batch="batch.csv")
@@ -303,3 +303,30 @@ def test_the_report_header_never_claims_apply_on_a_read_only_run(tmp_path, monke
 
     log = next(p for p in tmp_path.iterdir() if p.suffix == ".log")
     assert "mode        : DRY-RUN" in log.read_text(encoding="utf-8")
+
+
+# --- exit code --------------------------------------------------------------
+
+
+def _main_exit(api, batch_rows, tmp_path, monkeypatch, *args):
+    monkeypatch.setenv("CASEWORK_RUN_LOG_DIR", str(tmp_path))
+    monkeypatch.setattr(sr, "build_api", lambda a: api)
+    batch = tmp_path / "batch.csv"
+    batch.write_text("slug\n" + "".join(f"{s}\n" for s in batch_rows), encoding="utf-8")
+    return sr.main(["--batch-csv", str(batch), "--api-base-url",
+                    "http://127.0.0.1:48010", "--api-token", "t", *args])
+
+
+def test_a_clean_run_exits_zero(tmp_path, monkeypatch):
+    assert _main_exit(_StubApi(), [SLUG_A], tmp_path, monkeypatch, "--apply") == 0
+
+
+def test_a_run_with_any_failed_case_exits_non_zero(tmp_path, monkeypatch):
+    """A wrapper cannot otherwise tell a clean batch from one where every POST 500'd."""
+    api = _StubApi(errors={SLUG_A: _http_error(500)})
+    assert _main_exit(api, [SLUG_A, SLUG_B], tmp_path, monkeypatch, "--apply") == 1
+
+
+def test_skipped_cases_are_not_errors(tmp_path, monkeypatch):
+    api = _StubApi(reviews={SLUG_A: [{"id": 1841, "status": "done"}]})
+    assert _main_exit(api, [SLUG_A], tmp_path, monkeypatch, "--apply") == 0