feat(plugin): add EntityUpdateClientProtocol and parent-aware get

[SandyChapman]

Problem

The entity-client protocols in nemo_platform_plugin describe a narrower surface than EntityClient actually has, so a plugin needing more must declare a private protocol or type against the concrete class. Two capabilities are missing:

• update — the read-modify-write against the db_version optimistic lock.
• get(..., parent=...) — needed to address a child entity, which is unique within (workspace, entity_type, parent, name) rather than by name alone.

Approach

update becomes its own protocol, composed with the existing ones rather than folded into them:

class Store(EntityClientProtocol[T], EntityUpdateClientProtocol[T], Protocol[T]):
    ...

Folding it into EntityClientProtocol would be wrong: protocols are structural, so a new member silently invalidates every existing implementer — including every test double — even for services that never call it. MetricService only creates and reads; it shouldn't be typed against update, and its fakes shouldn't grow methods to satisfy a type.

parent is added to EntityGetterProtocol.get rather than given its own protocol, because a second protocol declaring a conflicting get couldn't compose with the first. It's optional, so fetching a root entity is unchanged.

That does require the existing test doubles to accept it — 10 fakes, one argument each. That's a correctness fix rather than churn: each stands in for a client that already accepts parent, so their signature was simply inaccurate.

Drift guard

The tests include a static conformance assertion:

def _static_conformance(client: EntityClient) -> _ReadWriteStore[_Entity]:
    return client

ty fails here if the client and protocols ever diverge. Verified it actually catches drift — renaming update in the protocol produces:

info: type `EntityClient` is not assignable to protocol `_ReadWriteStore[_Entity]`
info: └── protocol member `update_renamed` is not defined on type `EntityClient`

_ReadWriteStore is itself the composition shown above, so the test doubles as proof that the pieces compose.

Verification

• 1575 tests pass (nemo_platform_plugin + nemo-evaluator)
• tools/lint/lint-all.sh — 13/13
• ty across both packages: 130 diagnostics before, 128 after — two fewer, none new

Context

Prompted by review feedback on #1023, which had to declare a private EntityStoreProtocol to publish entity revisions. That PR will drop its private copy and compose these instead. No evaluator-specific behavior is included here — the only evaluator changes are the test-double signatures the protocol change requires.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Entity retrieval and deletion now support optional parent context for nested or child entities.
  • Added dedicated entity update support, including optimistic-locking safeguards.
  • Expanded the public API with additional entity client capabilities.

• Tests

  • Added coverage for parent-aware entity operations, update compatibility, and child-entity workflows.

[github-actions]

Suite	Lines Covered	Line Rate	Branch Rate
Unit Tests	30423/38448	79.1%	63.8%
Integration Tests	18022/37117	48.5%	21.0%

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8c6fa6da-4729-4b61-9e28-a1028a7693b2

📥 Commits

Reviewing files that changed from the base of the PR and between a6904bd and 70a4c17.

📒 Files selected for processing (15)

• packages/nemo_platform_plugin/src/nemo_platform_plugin/entities/__init__.py
• packages/nemo_platform_plugin/src/nemo_platform_plugin/entities/base.py
• packages/nemo_platform_plugin/src/nemo_platform_plugin/entity_client.py
• packages/nemo_platform_plugin/tests/entities/test_client_protocols.py
• packages/nemo_platform_plugin/tests/test_entity_client.py
• plugins/nemo-evaluator/tests/api/service/test_metric_service.py
• plugins/nemo-evaluator/tests/api/service/test_result_service.py
• plugins/nemo-evaluator/tests/api/service/test_task_service.py
• plugins/nemo-evaluator/tests/api/service/test_taskset_service.py
• plugins/nemo-evaluator/tests/api/v2/test_metrics_routes.py
• plugins/nemo-evaluator/tests/api/v2/test_results_routes.py
• plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py
• plugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.py
• plugins/nemo-evaluator/tests/test_metric_refs.py
• plugins/nemo-evaluator/tests/test_task_refs.py

🚧 Files skipped from review as they are similar to previous changes (14)

• packages/nemo_platform_plugin/src/nemo_platform_plugin/entities/init.py
• packages/nemo_platform_plugin/src/nemo_platform_plugin/entity_client.py
• packages/nemo_platform_plugin/tests/entities/test_client_protocols.py
• plugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.py
• packages/nemo_platform_plugin/src/nemo_platform_plugin/entities/base.py
• plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py
• plugins/nemo-evaluator/tests/test_task_refs.py
• plugins/nemo-evaluator/tests/api/service/test_task_service.py
• plugins/nemo-evaluator/tests/api/service/test_result_service.py
• plugins/nemo-evaluator/tests/test_metric_refs.py
• plugins/nemo-evaluator/tests/api/service/test_metric_service.py
• plugins/nemo-evaluator/tests/api/v2/test_metrics_routes.py
• plugins/nemo-evaluator/tests/api/v2/test_results_routes.py
• plugins/nemo-evaluator/tests/api/service/test_taskset_service.py

---

📝 Walkthrough

Walkthrough

The entity client contracts now support optional parent context for retrieval and deletion, plus a separate optimistic-locking update protocol. Public exports, protocol tests, client tests, and evaluator fakes match the revised interfaces.

Changes

Entity client contract alignment

Layer / File(s)	Summary
Entity retrieval and update contracts packages/nemo_platform_plugin/src/nemo_platform_plugin/entities/base.py	get and delete accept optional parent values. EntityUpdateClientProtocol defines asynchronous updates with optional original_name.
Public exports and protocol validation packages/nemo_platform_plugin/src/nemo_platform_plugin/entities/__init__.py, packages/nemo_platform_plugin/src/nemo_platform_plugin/entity_client.py, packages/nemo_platform_plugin/tests/entities/test_client_protocols.py, packages/nemo_platform_plugin/tests/test_entity_client.py	The update protocol is publicly exported. Tests validate protocol composition, signatures, parent forwarding, parent preservation, and update queries.
Evaluator test client alignment plugins/nemo-evaluator/tests/api/service/*, plugins/nemo-evaluator/tests/api/v2/*, plugins/nemo-evaluator/tests/test_*_refs.py	Test fakes accept optional parent arguments while retaining existing lookup and deletion behavior.

Possibly related PRs

• NVIDIA-NeMo/nemo-platform#731: Updates the shared entity-client and protocol surface.
• NVIDIA-NeMo/nemo-platform#951: Implements related parent-aware and optimistic-locking entity operations.
• NVIDIA-NeMo/nemo-platform#1023: Adds related revisioned, parent-scoped task and taskset workflows.

Suggested reviewers: mikeknep, jashg, marcusds

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely identifies the two main changes: EntityUpdateClientProtocol and parent-aware entity access.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch entity-store-client-protocol/schapman

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

packages/nemo_platform_plugin/tests/entities/test_client_protocols.py (1)

11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove postponed annotation evaluation.

from __future__ import annotations makes annotations string-based. All referenced types have normal imports. Remove this import.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/nemo_platform_plugin/tests/entities/test_client_protocols.py` at
line 11, Remove the `from __future__ import annotations` import statement at the
top of the test_client_protocols.py file. Since all type annotations in the
module use properly imported types rather than requiring string-based deferred
evaluation, this future import is unnecessary and should be deleted.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/nemo_platform_plugin/tests/entities/test_client_protocols.py`:
- Line 11: Remove the `from __future__ import annotations` import statement at
the top of the test_client_protocols.py file. Since all type annotations in the
module use properly imported types rather than requiring string-based deferred
evaluation, this future import is unnecessary and should be deleted.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 582832db-b1dd-4daf-8832-f8c52aa12bdd

📥 Commits

Reviewing files that changed from the base of the PR and between 2eb952f and 29eabaf.

📒 Files selected for processing (14)

• packages/nemo_platform_plugin/src/nemo_platform_plugin/entities/__init__.py
• packages/nemo_platform_plugin/src/nemo_platform_plugin/entities/base.py
• packages/nemo_platform_plugin/src/nemo_platform_plugin/entity_client.py
• packages/nemo_platform_plugin/tests/entities/test_client_protocols.py
• plugins/nemo-evaluator/tests/api/service/test_metric_service.py
• plugins/nemo-evaluator/tests/api/service/test_result_service.py
• plugins/nemo-evaluator/tests/api/service/test_task_service.py
• plugins/nemo-evaluator/tests/api/service/test_taskset_service.py
• plugins/nemo-evaluator/tests/api/v2/test_metrics_routes.py
• plugins/nemo-evaluator/tests/api/v2/test_results_routes.py
• plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py
• plugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.py
• plugins/nemo-evaluator/tests/test_metric_refs.py
• plugins/nemo-evaluator/tests/test_task_refs.py

[ironcommit]

We should probably follow up with further checks. delete also appears to be missing the parent field.

[SandyChapman]

Confirmed — and the audit turned up one more. Comparing each protocol method against EntityClient parameter by parameter:

get:    complete
create: complete
delete: protocol is missing ['parent']
list:   protocol is missing ['filter_obj', 'filter_str']

Folded the fixes into this PR:

• delete now takes parent.
• list deliberately keeps filter_str/filter_obj off, with a comment saying why: filter_operation is the sanctioned structured form and those two are a JSON-string variant and an exact-match shorthand kept for older callers.
• Added the further check you asked for. The existing conformance test could not have caught this: structural conformance is one-directional, so a class satisfies a protocol by providing at least what it declares and extra client parameters pass silently — ty reported success with all three parameters missing. The new test compares signatures parameter by parameter and fails on anything absent, with an explicit allowlist for the two intentional omissions. Reverting the delete fix makes it fail with protocol is missing client parameters: {'delete': {'parent'}}.

Full suite green (1576 passed); no new ty diagnostics in the touched files.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[SandyChapman]

One follow-on from reviewing this: update does not need parent, and adding it would be wrong.

The split is by whether the caller holds an entity. create and update read entity._parent (base.py:634, :742); get and delete address by name with nothing in hand, so they have to take it explicitly — which is why only those two had the gap. Giving update a parent argument would create two sources of truth and a silent conflict when they disagree.

That correctness argument rests on get populating _parent on the way out, and nothing was pinning it, so I added two tests to test_entity_client.py: a get→mutate→update round trip asserting the parent query param survives both hops, and a direct check that delete forwards it. Both mutation-tested — breaking either half of the round trip fails the test.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.