feat: migrate data review service to gRPC

[drake-nominal]

Moves the client's data review service from conjure to the v2 gRPC DataReviewService, following the pattern used for comments, units, workspace, sandbox workspace, and secrets.

Public method signatures are unchanged. DataReview gains _from_proto, and every RPC is wrapped in translate_grpc_errors(). _get_data_review and _initiate_data_review own their requests as well as their calls, so the four call sites that fetch or initiate a review share one path and Checklist.execute no longer builds a batch request by hand.

The v1 data review proto contract is not served by current backends. v2 replaces it and ships from nominal-api-protos 0.1354.0, which is already on main.

Completion no longer reads conjure private attributes. It previously tested state._pending_execution or state._executing; it now uses HasField over the evaluation-state oneof, which is the actual contract.

One behavior change is worth a look: Priority._from_proto distinguishes an unspecified priority, which returns None, from a level this build does not recognize, which raises. This matches _from_conjure. Collapsing both into None would make a future P5 read as no priority at all.

CheckViolation converts from the proto CheckAlert, which removes the last conjure import from the module.

🤖 Generated with Claude Code

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

[claude]

Claude finished @drake-nominal's task in 4m 37s —— View job

---

Review

• Gather context (diff, repo conventions, prior migrations)
• Structural lens pass
• Correctness / behavior-parity pass
• Tests & docstrings pass
• Post inline comments + overview

---

The public surface genuinely is unchanged, and the migration follows the shape the earlier grpc moves established (secret.py, containerized_extractor.py): translate_grpc_errors() around the RPC only, a module-level _get_data_review returning the raw proto so get_events and the three hydrating call sites share one path, _Clients protocol narrowed to the stub. The conjure surface is fully retired — no scout_datareview_api references remain anywhere in the repo.

Two things stand out as more than mechanical translation, both improvements:

• Replacing check.state._pending_execution or check.state._executing with HasField over the oneof. The old code was truthiness over conjure internals — it worked by accident of the union representation and would have gone quietly wrong if a state ever gained a falsy payload. The new form is the actual contract, and the pending/executing/settled/no-checks matrix is pinned in tests.
• _get_data_review in preference to four inline .get() calls. Returning the proto rather than a DataReview is the right boundary here — get_events needs the raw check_evaluations, and forcing it through the domain object would have meant either a second RPC or leaking evaluation state onto DataReview.

Findings, in rough priority order (all inline):

1. Priority._from_proto's case _: return None conflates "unspecified" with "a level this build doesn't recognize" — a future P5 would read as no priority, and _from_conjure raises for exactly that case. Suggested an explicit PRIORITY_UNSPECIFIED arm.
2. commit / asset_rid presence semantics — the one behavior change a MagicMock test can't reach. If those proto fields aren't optional, None becomes "" on the wire and "latest version" now depends on the backend treating empty as absent. Worth confirming against the proto.
3. _conjure_priority_to_priority is dead after this PR and was already a bare alias.
4. DataReviewBuilder.initiate is untested — the NotificationConfiguration positional→kwarg change plus tag fan-out across integrations is real mapping logic, not plumbing. Same for the ArchivedStatusSet wrapper, which is the only grpc search that wraps statuses in a message.
5. Duplicated BatchInitiate plumbing between Checklist.execute and DataReviewBuilder.initiate — the same extraction you already did for the get path applies. The RuntimeError message also still names batch_initiate.
6. Two ordering / docstring nits (_clientsbunch alphabetical block, two tests without the one-liner the rest of the file carries).

On the rebase note: leaving get_events' batch_get_events call on conjure is the right call for a single-service PR, and the test comment makes the flip obvious to whoever lands second.

I did not run tests, linters, or mypy — this was a static review, and CI covers those. One consequence worth stating: nominal.protos comes from the installed nominal-api-protos package, which isn't present in this checkout, so I could not verify proto field names or presence modifiers (ArchivedStatusSet.values, CheckAlert.end, CreateDataReviewRequest.commit). Finding 2 rests on that gap rather than on a confirmed defect.
• branch deidukas/migrate-datareview-to-grpc

[claude]

Claude finished @drake-nominal's task in 4m 45s —— View job

---

Review

• Fetch base, read diff and repo conventions
• Structural lens pass (thermo-nuclear)
• Correctness / behavior-parity pass
• Tests & docstrings pass
• Reconcile against prior review (👍 existing points, no restating)
• Post inline comments + overview

---

Second pass, against 2e8ab25. Six of the seven points from the first round are resolved in the tree: _conjure_priority_to_priority deleted, an explicit PRIORITY_UNSPECIFIED arm added ahead of a raising case _, _batch_initiate extracted, the RuntimeError message renamed, _clientsbunch ordering fixed, and the two missing test docstrings plus coverage for initiate's tag fan-out and the ArchivedStatusSet wrapper. The one still open — whether commit/asset_rid are optional in the v2 proto, so None doesn't become "" on the wire — I 👍'd rather than restate; it's the only behavior question a MagicMock stub can't reach, and nominal-api-protos still isn't in this checkout, so I can't settle it either.

Public surface and conventions. Unchanged, and the migration is clean against the pattern secret.py / containerized_extractor.py set: translate_grpc_errors() scoped to the RPC and nothing else, _Clients narrowed to the stub, module-level _-prefixed helpers taking _Clients as the first arg, paginate_grpc for the search path. The conjure surface is fully retired — no scout_datareview_api reference remains anywhere.

Two design calls are better than mechanical translation, worth naming once. HasField over the evaluation-state oneof replaces truthiness over conjure's _pending_execution / _executing internals — the old form worked by accident of the union representation and the new one is the actual contract, now pinned across pending/executing/settled/no-checks. And Priority._from_proto collapses the old inline is not scout_api.Priority.UNKNOWN ternary into one exhaustive mapping that every level is parametrized over, so the conversion stopped being a conditional at the call site.

Structurally nothing regresses: no file grows past a healthy boundary (data_review.py 269 lines, checklist.py 230), no branching is bolted onto an existing flow, and the diff net-removes conditionals. The helpers earn their keep rather than wrapping for its own sake.

New findings, all inline:

1. The _batch_initiate extraction stopped one step short — both call sites still repeat _from_proto(_get_data_review(...)) over the returned rids. Returning hydrated DataReviews collapses initiate to a line and Checklist.execute to a length check.
2. Checklist.execute is now the only rewritten RPC path with no unit coverage — its sole caller is an e2e test, and the len(rids) != 1 guard is real logic.
3. CheckViolation has no production constructor — _from_proto is reachable only from the two new tests, yet the type is in nominal.core.__all__. Pre-existing, but the PR is paying for a dead path.
4. The batch_get_events assertion the PR description says is commented for the feat: migrate event service to gRPC #913 rebase isn't commented.

One nit not worth its own thread: FindDataReviewsRequest(checklist_refs=[]) is vestigial — conjure required every field, proto3 doesn't, so that line can go.

Static review only — no tests, linters, or type-checkers run; CI covers those. The proto-presence gap above is the one place that limits what I could confirm.
• branch deidukas/migrate-datareview-to-grpc

[claude]

Claude finished @drake-nominal's task in 3m 46s —— View job

---

Review

• Fetch base, read diff and repo conventions
• Structural lens pass (thermo-nuclear)
• Correctness / behavior-parity pass
• Tests & docstrings pass
• Reconcile against prior rounds (👍, no restating)
• Post inline comments + overview

---

Third pass, against 68a9f80. The public surface is unchanged and the migration sits cleanly inside the pattern the earlier moves established (secret.py, containerized_extractor.py): translate_grpc_errors() scoped to the RPC and nothing else, _Clients narrowed to the stub, module-level _-prefixed helpers taking _Clients first, paginate_grpc for the search path. scout_datareview_api is gone from the repo, and _checklist_types now carries no conjure import at all.

Structurally nothing regresses. No file approaches a size boundary (data_review.py 293, checklist.py 230), no branching is bolted onto an existing flow, and the diff net-removes conditionals. Two calls are better than mechanical translation and worth naming once: _check_has_settled matching the evaluation-state oneof replaces truthiness over conjure's _pending_execution / _executing internals — the old form worked by accident of the union representation — and Priority._from_proto collapses the old inline is not scout_api.Priority.UNKNOWN ternary into one mapping every level is parametrized over, so the conversion stopped being a conditional at the call site. _initiate_data_reviews owning the BatchInitiateRequest shape (not just the call) is what lets Checklist.execute drop its data_review_pb2 import entirely.

Three new findings, all inline: the case None arm in _check_has_settled and what the docstring above it promises; a test docstring that describes a _get_data_review contract the function doesn't have; a missing Returns: on initiate.

Reconciliation with prior rounds. Most of round 1 and 2 landed. Two replies don't match the tree, so I 👍'd the originals rather than restate them:

• _get_data_review still returns data_review_pb2.DataReview, and all three _from_proto(_get_data_review(...)) sandwiches are still there (client.py:1302, data_review.py:93, data_review.py:281). The reply said the sandwiches were gone and a _proto variant added for get_events; neither is at HEAD. Every other _get_* in the repo (_get_containerized_extractor, etc.) returns the domain object, so this is the one place the migration diverges from the house shape.
• CheckViolation._from_proto still has no caller outside tests/core/test_data_review.py:178, and the class is still in nominal.core.__all__. Pre-existing, unanswered.

Also: the PR description says Priority._from_proto "raises" for a level this build doesn't recognize. The code returns None, with a docstring and a test (id="a-level-added-after-this-client-was-built") arguing for that deliberately. The reasoning is sound — the conjure decoder did collapse unrecognized values into UNKNOWN, which the old call site read as absence — but the description now describes the round-1 behavior, not the code.

Verification gap: nominal-api-protos isn't installed in this checkout, so I could not read the generated stubs or descriptors. The claim about WhichOneof being typed as a Literal union follows from how mypy-protobuf generates stubs generally, not from reading this build's .pyi. Static review only — no tests, linters, or type-checkers run; CI covers those.
• branch deidukas/migrate-datareview-to-grpc

[claude]

case None covers two different things: a check with no state set, and — for a client built against an older proto — a state the server added since. Unknown oneof arms land in the unknown-field set and never populate the oneof, so WhichOneof returns None there too. Both read as settled, so poll_for_completion can return a review whose checks are still running.

That matches the conjure code, so it isn't a regression. But the docstring above promises the opposite: the exhaustiveness it describes is a type-check-time property (mypy-protobuf types WhichOneof as a Literal union, so a regenerated proto with a new arm fails the build) and buys nothing at runtime before that regeneration. Worth scoping the claim to what it actually guarantees.

[claude]

The docstring describes a contract the helper doesn't have. _get_data_review returns data_review_pb2.DataReview; the hydration this test observes is NominalClient.get_data_review's, and the _from_proto(_get_data_review(...)) sandwich is still repeated at client.py:1302, data_review.py:93, and data_review.py:281. A reader trusting the docstring would drop their _from_proto and get a proto back.

[claude]

No Returns: block, though the return is the whole point of the method and wait_for_completion changes what you get back (settled reviews vs. reviews that may still be running). execute_checklist directly above documents its return.