Added critic

[crystqlline]

No description provided.

[xTRam1]

Senior Review: SDK Critic Feature

Overall this is a solid feature addition — the critic pattern is clean and the CriticConfig / CriticResult public API is well-designed. A few things need attention before merge:

1. run_critic is business logic living in a models file (High)

run_critic() is an ~50-line async function with orchestration logic (building dynamic Pydantic models, dispatching requests, parsing results). It lives in narada_core/actions/models.py, which is a data models module.

This leaks an abstraction — anyone importing from actions/models.py expects data classes, not orchestration functions. Consider moving run_critic to its own module (e.g., narada_core/actions/critic.py or narada_core/critic.py) and keeping CriticResult in the models file where it belongs.

2. Unused import uuid in packages/narada/src/narada/window.py (Medium)

The diff adds import uuid to narada/window.py, but run_critic (which uses uuid) was moved to narada_core/actions/models.py. This import is dead code and will get flagged by linters.

3. assert critic_content is not None is a crash in production (High)

In run_critic(), line:

critic_content = critic_dispatch_response["response"]
assert critic_content is not None

assert statements are stripped in optimized mode (python -O). If the critic dispatch ever returns a None response (network issue, timeout, malformed response), this silently passes in production and then crashes on the next line with an unhelpful TypeError. Replace with an explicit check:

if critic_content is None:
    raise ValueError("Critic dispatch returned no response")

4. _VALIDATION_VAR naming collision risk is low but the randomness is unnecessary complexity (Low)

_VALIDATION_VAR = f"narada_validation_passed_{uuid.uuid4().hex[:4]}"

The 4-char hex suffix creates a different field name every call, which means the backend schema is non-deterministic. If you're worried about collision with user-defined output_schema fields, a fixed reserved name like __narada_validation_passed__ with dunder convention is cleaner and more predictable. If a user names their field that, that's on them — the dunder convention clearly signals "internal."

5. model_config = ConfigDict(arbitrary_types_allowed=True) on CriticResult — is it needed? (Low)

CriticResult fields are: bool, str, Any, AgentUsage, ActionTrace | None. The Any on structured_output might warrant it, but it's worth checking if this is actually needed. If not, removing it tightens the model validation.

6. Duplication across narada/window.py and narada-pyodide/window.py (Medium)

The agent() method in both narada/window.py and narada-pyodide/window.py has identical critic integration code (the if critic is not None: critic_result = await run_critic(...) block). Since run_critic already lives in narada-core, this is good, but the 12-line call-site block is duplicated verbatim. If either signature changes, both files must be updated. Not blocking, but worth noting — if the Window base in narada-core ever gets an agent() method, this should be consolidated.

7. No tests (Blocking concern)

This is 179 lines of new code including async orchestration logic, dynamic Pydantic model creation, and structured output parsing. There are zero tests. At minimum:

• Unit test for run_critic with a mocked dispatch_request
• Test that CriticConfig with and without output_schema both work
• Test that the validation variable is correctly extracted and removed from structured output
• Edge case: structured_output is None when output_schema is provided

Minor nits

• The three overloads of dispatch_request in both window files all get critic_context added — clean and consistent, looks good.
• CriticConfig defaults are sensible (default prompt, optional schema, optional MCP servers).
• Re-exporting CriticConfig and CriticResult from narada/__init__.py — good for public API discoverability.

Verdict: The API design is clean, but the assert-in-production bug and lack of tests should be resolved before merge. The models-file placement is a design smell worth fixing now while the surface area is small.