diff --git a/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/README.md b/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/README.md new file mode 100644 index 0000000000..1e191d8bac --- /dev/null +++ b/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/README.md @@ -0,0 +1,40 @@ +# Invoke validation + +Implemented. Validation lands at the SDK resolver boundary (see `status.md`). + +The agent service invoke endpoint (`POST {host}/services/agent/v0/invoke`) did not tell a +caller when the request was malformed. Send `references` with no revision, or send +`data.revision` one level too shallow, and the service silently ran a seeded default +(`pi_core` / `gpt-5.5`) and then 500'd. The caller got an opaque late error instead of "your +request was shaped wrong, here is the right shape." + +This project reframes that as a validation problem. The boundary validates the request up front +and, when it cannot resolve to a config, returns a clear 400 that names the two valid ways to +invoke an application: + +1. Provide configuration inline (`data.parameters`). +2. Provide a revision: either a complete revision nested correctly (`data.revision = {"data": + ...}`), or a resolvable reference that pins one committed config (a variant, an environment, or + a revision, not a bare application). + +## Files + +- `context.md` — the symptom (malformed invoke -> silent default -> late 500), why it cost the + lab time, and why validation (not self-hydration) is the right lens. Goals and non-goals. +- `research.md` — the mechanism with `file:line` citations: the resolver requires a double-nested + revision (`resolver.py:150`), the agent's seeded default disables reference hydration + (`utils.py:285-287`, gate at `resolver.py:573-577`), the envelope ignores unknown fields + (`workflows.py:237`, `:296`), the product path pre-hydrates the reference so it works + (`service.py:745-751`), the reference-family validator that already raises 4xx + (`resolver.py:69-98`), the three reference-kind results, and the live reproduction. +- `plan.md` — the shipped validation: Rules A (revision nested right) and B (reference must be + resolvable, not a bare application), raised as a clear 400 at the shared resolver boundary, + consistent across agent / completion / chat. Scope limits (no blanket forbid, no OpenAPI, no + "nothing to run" rule so completion / chat do not regress) and the test matrix. +- `status.md` — implemented; decisions locked and open questions resolved. + +## Status + +Implemented at the SDK resolver boundary (`_validate_resolvable_config`). Supersedes and merges +the earlier `harden-invoke` decision and the `silent-fallback` / `invoke-contract` threads under +`docs/design/agent-workflows/scratch/console/builder-kit/`. See `status.md`. diff --git a/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/context.md b/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/context.md new file mode 100644 index 0000000000..a08779877b --- /dev/null +++ b/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/context.md @@ -0,0 +1,87 @@ +# Context: validate the invoke request instead of silently defaulting + +## The symptom + +A caller sends a malformed request to the agent service invoke endpoint +(`POST {host}/services/agent/v0/invoke`). The endpoint does not tell the caller the request +is malformed. It silently runs a seeded default agent (`pi_core` / `gpt-5.5`), and then that +default 500s a few steps later because `gpt-5.5` needs a provider prefix. The caller sees a +late, confusing 500. Nothing points back at the real cause: the request was shaped wrong. + +Two malformed shapes both hit this trap: + +- **References only, no revision.** The caller passes `references` (for example an + `application_revision` ref) and expects the service to fetch and run the committed config. + It does not. The service runs the seeded default and 500s. +- **A revision nested one level too shallow.** The caller passes + `data.revision = ` (the bare revision fields), when the resolver requires the + double-nested `data.revision = {"data": }`. The wrong nesting is ignored, the + seeded default runs, and it 500s. + +Both are the same failure from the caller's point of view: I sent config, the run ignored it, +and I got an opaque 500 with the wrong model in the trace. + +## Why this cost the lab time + +The agent-creation lab drives the low-level service endpoint directly (it does not go through +the product API). During the lab, guessing the right invoke shape was slow and expensive +because a wrong guess did not fail loudly. It returned a 200-then-500 with a default config in +the resolved trace, which looks like a runtime bug in the agent rather than a bad request. The +operator had to reverse-engineer the exact nesting from the resolver source to find the one +working shape. A single clear 4xx at the boundary would have replaced that whole investigation. + +## Why validation is the right lens + +An earlier framing called this a "self-hydration" gap: the service should fetch the reference +itself the way completion and chat do. That fix is real and worth doing (see the sibling +finding on the seeded-default hydration gate), but it is not the frame the user wants for this +project. The user's frame is simpler and more durable: + +> This is a validation problem. The endpoint should check the request up front and, when the +> request cannot resolve to a config, return a clear error that names the valid ways to call +> the application. + +Under that frame there are exactly two valid ways to invoke — there is no legitimate empty or +default intent, so the caller either gives a config or specifies a revision: + +1. **Provide configuration inline** (`data.parameters = {"agent": {...}}`). +2. **Provide a revision**: either a complete revision nested correctly (`data.revision = {"data": + ...}`), or a resolvable reference the service can fetch a specific committed config from. + +And a key rule inside option 2: a bare `application` reference is not enough. An application +holds many variants and revisions, so `application` alone cannot pin one config. A resolvable +reference needs at least a variant, an environment, or a revision. The endpoint validates that +too. + +The goal: a caller that sends the data the wrong way gets a precise error that explains the +right way, instead of a silent default followed by a late 500. + +## Goals + +- Turn a malformed invoke into a clear 400 at the boundary, before any run starts. +- Make the error name the two valid call shapes and, for references, name the missing + identity (variant, environment, or revision). +- Validate that a supplied revision is nested correctly. +- Keep the behavior consistent across the agent, completion, and chat services where the + contract is shared. + +## Non-goals + +- **Not OpenAPI.** The decision to keep the services' OpenAPI off stands (a bare `Optional[dict]` + parameters field renders as an opaque, misleading schema; `/inspect` is the live contract). + See the superseded `harden-invoke` decision. +- **Not blanket `extra="forbid"` on the whole envelope.** The user rejected locking the request + shape down wholesale. Validation should fail loud with a good message on the load-bearing + fields (is there a resolvable target, is a supplied revision nested right), not reject every + unknown field. +- **Not a rewrite of reference resolution.** This project validates the request shape at the + boundary. Whether the service also self-hydrates a bare reference (the seeded-default gate) is + tracked as related work and can land alongside, but the validation is the primary deliverable. + +## Relationship to earlier notes + +This project supersedes and merges the earlier `harden-invoke` decision and the `silent-fallback` +and `invoke-contract` threads under +`docs/design/agent-workflows/scratch/console/builder-kit/`. Those converged on "add strict +validation with clear errors, keep OpenAPI off." This workspace is the durable home for that +conclusion, reframed around request validation and the two valid call shapes. diff --git a/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/plan.md b/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/plan.md new file mode 100644 index 0000000000..e32ba785d0 --- /dev/null +++ b/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/plan.md @@ -0,0 +1,138 @@ +# Plan: validate the invoke request at the boundary + +## The core idea + +Validate the invoke request before any run starts. When the request cannot resolve to a config, +return a 400 whose message names the two valid ways to call an application. Do this in the shared +SDK resolver boundary that already validates reference families, so the behavior is consistent +across the agent, completion, and chat services. + +The rule the boundary enforces: **a well-formed invoke supplies its config in one of two ways.** +There is no legitimate empty or default intent — the caller either gives a config or specifies a +revision. + +1. **Inline configuration:** `data.parameters = {"agent": {...}}`. +2. **A revision.** Either: + - a complete revision, nested correctly: `data.revision = {"data": }`; or + - a resolvable reference that pins one committed config: a variant, an environment, or a + revision (`latest` is fine), not a bare application. + +If neither holds, the request is malformed and gets a clear error instead of a silent default and +a late 500. + +## What "validate" means, precisely + +The check runs at the resolver boundary, right where `_validate_executable_reference_families` +already runs (`sdks/python/agenta/sdk/middlewares/running/resolver.py:69-98`). It answers one +question: can this request resolve to a caller-intended config? It raises `bad_request` (400) with +a specific message when it cannot. It ships as a sibling validator, `_validate_resolvable_config`, +called at the top of `ResolverMiddleware.__call__`, so it does not disturb the family check or the +`resolve_references_with_info` path. + +### Rule A: a revision, if present, must be nested correctly + +If `data.revision` is present, it must be the double-nested shape the resolver reads +(`data.revision["data"]`, `resolver.py:150`). If `data.revision` is a dict but has no `data` +key, the caller almost certainly sent the bare revision fields one level too shallow. Reject +with a message that shows the required shape: + +> `data.revision` must be nested as `{"data": {...}}` (the revision's own fields go under +> `data`). You sent the fields at the top level. Wrap them: `data.revision = {"data": +> }`. + +This turns the silent single-nested failure (live result: HTTP 500, `gpt-5.5`) into an +immediate, self-explaining 400. + +### Rule B: a reference, if it is the intended target, must be resolvable + +If the request supplies `references` but no `data.revision` and no inline `data.parameters`, +then the reference is the intended config source. Validate that the reference can pin one +committed config: + +- A bare `application` reference (no variant, no environment, no revision) is not resolvable. + An application has many revisions; nothing selects which one to run. Reject with: + + > A bare `application` reference cannot select a config. An application has many variants and + > revisions. Provide one of: an `application_variant` reference, an `environment` reference, + > or an `application_revision` reference. + +- The same rule applies per family (workflow, evaluator) using their variant/revision members. + +This reuses the family grouping already in `_validate_executable_reference_families`. That +function proves the pattern: it groups references into `workflow` / `application` / `evaluator` +families and raises a clear `bad_request`. Rule B extends it from "exactly one family" to "and +that family names a resolvable target." + +### No "nothing to run" rule for the empty body + +An earlier draft added a Rule C that rejected a request with no revision, no reference, and no +inline parameters. It is dropped. An empty request expresses no config intent, and config can +still arrive from the running context or a pre-installed handler, so rejecting it at the shared +boundary would regress completion and chat (and the local workflow path). The validator therefore +rejects only an EXPRESSED but unresolvable intent: a present single-nested `data.revision` +(Rule A) or references that pin nothing (Rule B). That covers the two shapes the lab actually hit +— a wrong nesting and a references-only call with no committed target — while leaving the empty +body alone. + +## Scope of the strictness (what we are NOT doing) + +- **No blanket `extra="forbid"` on the envelope.** Rules A and B fail loud on the load-bearing + decision (is there a resolvable config), not on every unknown field. A caller can still attach + extra metadata. +- **No OpenAPI.** Keeping the services' OpenAPI off stands. `/inspect` is the live contract. This + plan does not touch that decision. +- **The self-hydration fix is separate and complementary.** Removing the agent's seeded + parameters (`utils.py:285-287` -> `WorkflowRevisionData()`) would make a references-only agent + call self-hydrate like completion and chat. That is a real fix, tracked as a follow-up, and is + NOT bundled here. Validation is the deliverable because it helps even when self-hydration is + intentionally off: a caller who sends a wrong nesting or a bare application still gets a clear + error. + +## Consistency across agent / completion / chat + +The three services share the resolver, so Rules A and B live in the shared boundary and all three +get the same clear 400 for a wrong nesting or a bare application. The validator inspects only what +the caller sent, not what the resolver would fall back to, so it does not depend on the +agent-specific seeded default. Because there is no "nothing to run" rule, completion and chat keep +their empty-body behavior unchanged. + +## Error model + +Use the existing `bad_request` helper the family validator already uses (`_raise_bad_request`, +`resolver.py`), which sets `status_code = 400`. The message names which rule failed, shows the +correct shape (Rule A) or explains why the reference is not resolvable (Rule B), and lists the two +valid call shapes via the shared `_INVOKE_CALL_SHAPES` tail. + +## Where the code changes landed + +- `sdks/python/agenta/sdk/middlewares/running/resolver.py`: `_validate_resolvable_config` + (Rules A and B) plus `_RESOLVABLE_REFERENCE_KEYS` and `_INVOKE_CALL_SHAPES`, sitting next to + `_validate_executable_reference_families` and called at the top of `ResolverMiddleware.__call__`. + One boundary, all three services. +- `sdks/python/oss/tests/pytest/utils/test_resolver_middleware.py`: `TestResolverConfigValidation`. +- Not touched: `sdks/python/agenta/sdk/models/workflows.py` (the nesting check stays in the + resolver) and `services/oss/src/agent/app.py` (the shared message is clear enough). + +## Supersedes / merges + +This plan supersedes and merges the earlier `harden-invoke` decision and the `silent-fallback` / +`invoke-contract` threads under +`docs/design/agent-workflows/scratch/console/builder-kit/`. Their conclusion (add clear-error +validation, keep OpenAPI off, do not blanket-forbid) is carried forward here and reframed around +request validation and the two valid call shapes. + +## Test plan (shipped) + +Unit tests in `TestResolverConfigValidation` +(`sdks/python/oss/tests/pytest/utils/test_resolver_middleware.py`): + +- References only, bare `application` -> 400 naming variant/environment/revision (Rule B). Also a + bare `workflow` root -> 400. +- References only, a resolvable variant / environment / revision -> passes (no reject). +- Single-nested `data.revision` -> 400 with the nesting fix (Rule A). +- Double-nested `data.revision` -> passes (no reject). +- Inline `data.parameters` -> passes (no reject). +- Empty body -> passes (no "nothing to run" rule; completion / chat do not regress). +- The bare-application reject also fires through `ResolverMiddleware`, before `call_next`. + +Capture a live pass with a replay test (see the `agent-replay-test` skill) once deployed. diff --git a/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/research.md b/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/research.md new file mode 100644 index 0000000000..0a81c8a947 --- /dev/null +++ b/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/research.md @@ -0,0 +1,190 @@ +# Research: why a malformed invoke silently defaults + +All paths below were read on the `big-agents` line. Line numbers are current as of +2026-07-01. + +## The invoke path in one picture + +`POST {host}/services/agent/v0/invoke` reaches the shared SDK running layer. A middleware +resolves the config to run, then the agent handler builds its template from that config. Three +services (agent, completion, chat) share this layer; they differ only in one seeded default. + +The config the run uses is chosen by `resolve_revision(...)` and then adjusted by +`ResolverMiddleware.__call__(...)`, both in +`sdks/python/agenta/sdk/middlewares/running/resolver.py`. + +## 1. The resolver requires a double-nested revision + +`resolve_revision(...)` prefers a caller-supplied revision from `request.data.revision`: + +- `resolver.py:147-152`: + ```python + if request and request.data and request.data.revision: + rev_dict = request.data.revision + # revision dict is the full WorkflowRevision dump; data sub-key holds the actual fields + data_dict = rev_dict.get("data") if isinstance(rev_dict, dict) else None + if data_dict: + return WorkflowRevisionData(**data_dict) + ``` + +The branch reads `rev_dict.get("data")` (`resolver.py:150`) and builds `WorkflowRevisionData` +from it (`resolver.py:152`). So the required shape is: + +```json +"data": { "revision": { "data": { "uri": ..., "parameters": ... } } } +``` + +A caller who sends the bare revision fields (`data.revision = {"uri": ..., "parameters": ...}`) +has no nested `data` key, so `data_dict` is falsy, the branch is skipped, and the resolver falls +through to the seeded default. The wrong nesting produces no error; it produces a default. + +Note the strict twin already exists: `WorkflowRevisionData` sets `model_config = +ConfigDict(extra="forbid")` (`sdks/python/agenta/sdk/models/workflows.py:152-153`). The revision +body is strictly validated, but only once the resolver reaches it. Wrong nesting means the +strict model never runs. + +## 2. The seeded default disables reference hydration for the agent + +When there is no usable `data.revision`, `resolve_revision` returns +`RunningContext.revision` (`resolver.py:154-162`), which the decorator seeded from the workflow's +registered default. For the agent workflow that default is not empty: + +- `sdks/python/agenta/sdk/engines/running/utils.py:285-287`: + ```python + agent=dict( + v0=WorkflowRevisionData(parameters={"agent": build_agent_v0_default()}) + ), + ``` + `build_agent_v0_default()` is `pi_core` / `gpt-5.5`. + +- Compare chat and completion, which seed empty data: + `utils.py:279-280` → `chat=dict(v0=WorkflowRevisionData())`, + `completion=dict(v0=WorkflowRevisionData())`. No parameters. + +The hydration gate then behaves differently for the agent than for chat/completion: + +- `resolver.py:572-577`: + ```python + request_has_parameters = bool(request.data and request.data.parameters) + needs_reference_hydration = bool( + request.references + and not request_has_parameters + and (revision is None or not revision.parameters) # seeded default HAS parameters + ) + ``` + +For the agent, the seeded default's `parameters` are non-empty, so +`revision is None or not revision.parameters` is False. `needs_reference_hydration` is False. +The reference is never fetched. The seeded default runs. For chat and completion the seed is +empty, the gate is True, and the reference is fetched and applied. + +Net effect at the service: a references-only agent invoke runs `pi_core` / `gpt-5.5` and then +500s, because that model needs a provider prefix. + +## 3. The request envelope ignores unknown fields + +Even before the resolver, the request models accept junk quietly: + +- `WorkflowRequestData` (`workflows.py:237-239`) has `revision: Optional[dict]` and + `parameters: Optional[dict]`, and does not set `extra="forbid"`. A misspelled top-level field + (for example `parameter` instead of `parameters`, or `revisions` instead of `revision`) is + dropped, leaving the field `None`. +- `WorkflowInvokeRequest` (`workflows.py:296-297`) likewise does not forbid extras. +- `parameters` is an untyped dict, so the agent config inside it is never schema-checked at the + envelope. + +So a wrong field name and a wrong nesting both survive as "no config supplied," and the run +proceeds on the seeded default. The precedent for strictness exists in the same file +(`WorkflowRevisionData` at `workflows.py:152-153` uses `extra="forbid"`), it is just not applied +to the envelope. + +## 4. Why the product path works but a direct service call does not + +The product never sends a bare reference to the service. The API resolves the reference first +and forwards the double-nested revision: + +- `api/oss/src/core/workflows/service.py:745-751`: + ```python + if workflow_revision and workflow_revision.data: + if not request.data: + request.data = WorkflowRequestData() + request.data.revision = { + "data": workflow_revision.data.model_dump(mode="json") + } + ``` + +`_ensure_request_revision` runs inside `_prepare_invoke`, and every product invoke caller +(triggers, HITL respond, sessions, evaluations, tools) routes through it. So the service always +receives the resolved, double-nested `data.revision`, and the committed config runs. The gap is +only visible when a caller talks to the service directly, which is exactly what the lab does. + +## 5. Reference kinds: a bare application is not resolvable + +Reference resolution lives in `resolve_references_with_info(...)` +(`sdks/python/agenta/sdk/middlewares/running/resolver.py`, around 240-460). Two facts matter for +validation. + +**There is already a boundary validator, and it already raises 4xx.** +`_validate_executable_reference_families(refs)` (`resolver.py:69-98`) groups references into +families and rejects competing targets: + +```python +if len(populated) > 1: + _raise_bad_request( + "Competing execution target references are not allowed. " + "Provide exactly one of workflow, application, or evaluator " + f"references; got {', '.join(populated)}." + ) +``` + +This is the natural home for the new check, and it proves the pattern: validate the reference +shape at the boundary and raise a clear `bad_request`. + +**The application family has three identity levels, and only the deeper two pin a config.** The +mapping the resolver builds (`resolver.py`, application mapping) is: + +- `application_ref` -> `application` +- `application_variant_ref` -> `application_variant` +- `application_revision_ref` -> `application_revision` + +with a parallel `environment` family (`environment_ref`, `environment_variant_ref`, +`environment_revision_ref`). + +The three reference-kind results: + +| Reference supplied | Can it pin one committed config? | Why | +| --- | --- | --- | +| `application` only (bare app) | No | An application has many variants and revisions. A bare app id is ambiguous; nothing selects which revision to run. The retrieve call gets only `application_ref` and cannot resolve a single revision. | +| `application_variant` (or `environment`) | Yes | A variant selects its current revision; an environment selects the revision deployed to it. `environment_backed_application_lookup` (`resolver.py:348-356`) uses the environment mapping to pick the target. | +| `application_revision` | Yes | Names the exact committed revision. `resolve_references_with_info` returns `WorkflowRevisionData(**revision["data"])` for it. | + +So "provide a resolvable reference" means: provide a variant, an environment, or a revision, not +a bare application. Today the resolver does not reject a bare application; it just fails to +resolve and (for the agent) falls through to the seeded default. + +## 6. Live results that confirm the mechanism + +From `docs/design/agent-workflows/scratch/console/builder-kit/findings/reference-invoke-definitive.md` +(reproduced live against `https://bighetzner.agenta.dev`): + +- **Double-nested `data.revision = {"data": }`** -> HTTP 200, resolved + `harness=claude, model=sonnet` (the committed config). Works. +- **References only (no `data.revision`, no `parameters`)** -> HTTP 500, resolved `gpt-5.5`. The + seeded default ran; the reference was never hydrated (gate at `resolver.py:573-577`). +- **Single-nested `data.revision = `** -> HTTP 500, resolved `gpt-5.5`. + `rev_dict.get("data")` was None, the branch was skipped, the seeded default ran. + +The third working option is inline: `data.parameters = {"agent": {...}}`, which takes the +`request.data.parameters` branch directly (`resolver.py:605-610`) and never consults the seed. + +## Files that changed + +- `sdks/python/agenta/sdk/middlewares/running/resolver.py` — `_validate_resolvable_config` (the + boundary validator, next to `_validate_executable_reference_families`), called from + `ResolverMiddleware.__call__`. +- `sdks/python/oss/tests/pytest/utils/test_resolver_middleware.py` — `TestResolverConfigValidation`. + +Not touched (deliberately): `sdks/python/agenta/sdk/models/workflows.py` (the revision-nesting +check stays in the resolver, not the model) and `services/oss/src/agent/app.py` (the shared +message is clear enough). The seeded-default self-hydration fix (`utils.py:285-287`) is a separate +follow-up, not bundled here. diff --git a/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/status.md b/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/status.md new file mode 100644 index 0000000000..452eb0aa86 --- /dev/null +++ b/docs/design/agent-workflows/projects/builder-agent-reliability/invoke-validation/status.md @@ -0,0 +1,71 @@ +# Status + +**State: implemented. Validation lands at the SDK resolver boundary.** + +This workspace reframed the invoke silent-fallback problem as a request-validation problem and +now ships the validation. It supersedes and merges the earlier `harden-invoke` decision and the +`silent-fallback` / `invoke-contract` threads under +`docs/design/agent-workflows/scratch/console/builder-kit/`. + +## What is decided + +- The frame is validation, not self-hydration. A malformed invoke gets a clear 400 at the + boundary, not a silent default and a late 500. +- **Two valid call shapes, not three.** There is no legitimate empty or default intent for an + agent invoke. The caller either provides a config, or specifies a revision: + 1. **Inline configuration:** `data.parameters`. + 2. **A revision:** either a correctly double-nested `data.revision = {"data": {...}}`, or a + resolvable reference that pins one committed config — a variant, an environment, or a + revision (`latest` is fine; an environment selects the deployed revision). +- A bare `application` reference is not resolvable; the same holds for a bare `workflow` or + `evaluator` root. A resolvable reference needs a variant, an environment, or a revision. +- **Status code: 400 (`bad_request`).** Matches `_validate_executable_reference_families`, which + already raises 400 for competing reference families at the same boundary. Stay consistent. +- **Where it lives: the SDK resolver boundary only.** A sibling validator, + `_validate_resolvable_config`, sits next to `_validate_executable_reference_families` in + `sdks/python/agenta/sdk/middlewares/running/resolver.py` and is called at the top of + `ResolverMiddleware.__call__`. The revision-nesting check lives in the resolver (Rule A), not in + `models/workflows.py`, so the error can carry the same shapes message. +- **Scope to the config path, not the whole envelope.** No blanket `extra="forbid"`. The + validator only rejects an expressed-but-unresolvable config intent (a wrong revision nesting, or + a reference that pins nothing). Extra metadata fields are still accepted. No OpenAPI; `/inspect` + stays the live contract. +- **The self-hydration fix stays separate.** Dropping the agent's seeded parameters at + `utils.py:285-287` (so a references-only agent call self-hydrates like completion and chat) is a + complementary follow-up and is NOT bundled here. This change is validation-only. + +## How the open questions resolved + +1. **400 vs 422.** 400 (`bad_request`), to match the existing family validator at the same + boundary. +2. **Rule C over-rejection on completion/chat.** Dropped as its own rule. The validator never + rejects an empty body: an empty request expresses no config intent, and config can still arrive + from the running context or a pre-installed handler. It rejects only an EXPRESSED but + unresolvable intent — a present-but-single-nested `data.revision` (Rule A) or references that + pin nothing (Rule B). So completion and chat do not regress, and there is no "run the default" + carve-out to reason about. +3. **Where the check lives.** A sibling validator (not folded into the family check), at the + resolver boundary. Simpler to read and keeps the family check focused. +4. **Revision-nesting check placement.** In the resolver (Rule A), so the error references the + same two-shapes message. Not in `models/workflows.py`. +5. **Bundle with self-hydration?** No. Validation-only. The seed fix is tracked separately. + +## What shipped + +- `sdks/python/agenta/sdk/middlewares/running/resolver.py`: `_validate_resolvable_config` (Rules A + and B) plus the `_RESOLVABLE_REFERENCE_KEYS` / `_INVOKE_CALL_SHAPES` helpers, called from + `ResolverMiddleware.__call__`. +- `sdks/python/oss/tests/pytest/utils/test_resolver_middleware.py`: `TestResolverConfigValidation` + covering inline config, a double-nested revision, a resolvable reference (variant / environment / + revision), a bare `application` reject, a bare `workflow` reject, a single-nested `data.revision` + reject, an empty-request pass, and a bare-application reject through the middleware. + +## Next steps + +- Complementary follow-up: drop the agent's seeded default parameters (`utils.py:285-287` -> + empty `WorkflowRevisionData()`) so a references-only agent call self-hydrates like completion and + chat. Tracked separately. +- Capture a live pass as a replay test once the change is deployed (see the `agent-replay-test` + skill). +- The lab kit already documents inline and the double-nested revision as the correct shapes; keep + it in sync if the messages change. diff --git a/sdks/python/agenta/sdk/middlewares/running/resolver.py b/sdks/python/agenta/sdk/middlewares/running/resolver.py index c44e2a3c6e..32fbd37931 100644 --- a/sdks/python/agenta/sdk/middlewares/running/resolver.py +++ b/sdks/python/agenta/sdk/middlewares/running/resolver.py @@ -99,6 +99,103 @@ def _validate_executable_reference_families(refs: Dict[str, Any]) -> None: ) +# Reference keys that pin exactly one committed config, so they can resolve an +# invoke. A bare family root (``application`` / ``workflow`` / ``evaluator``) is +# NOT here: a root names many revisions and selects none. An ``environment`` root +# IS resolvable — it pins the revision deployed to that environment. +_RESOLVABLE_REFERENCE_KEYS = ( + "workflow_variant", + "workflow_revision", + "application_variant", + "application_revision", + "evaluator_variant", + "evaluator_revision", + "environment", + "environment_variant", + "environment_revision", +) + +# Shared tail for both invoke-validation errors: the two valid call shapes, plus +# why a bare ``application`` reference cannot resolve. Kept as one constant so the +# Rule A (nesting) and Rule B (reference) messages stay in sync. +_INVOKE_CALL_SHAPES = ( + "An invoke must resolve to a config in one of two ways: " + "(1) inline configuration at `data.parameters`; or " + "(2) a revision reference that pins one committed config — an " + "`application_variant` (or `workflow_variant` / `evaluator_variant`), an " + "`environment`, or an `application_revision` (or `workflow_revision` / " + "`evaluator_revision`). A bare `application` reference is not resolvable: an " + "application has many revisions and nothing selects which one to run. A " + 'supplied `data.revision` must be double-nested as `{"data": {...}}`.' +) + + +def _is_double_nested_revision(revision: Any) -> bool: + """True when ``data.revision`` carries its fields under a nested ``data`` key. + + This is the only shape ``resolve_revision`` consumes (``resolver.py:150``): it + reads ``rev_dict["data"]``. A single-nested revision (the bare revision fields + at the top level) has no ``data`` key, so the resolver drops it silently and + the run falls through to a seeded default. Rule A rejects that shape instead. + """ + return bool(isinstance(revision, dict) and revision.get("data")) + + +def _has_resolvable_reference(refs: Dict[str, Any]) -> bool: + return any( + _has_reference_identity(refs.get(key)) for key in _RESOLVABLE_REFERENCE_KEYS + ) + + +def _validate_resolvable_config(request: WorkflowInvokeRequest) -> None: + """Reject an invoke that cannot resolve to a caller-intended config. + + Runs at the resolver boundary, alongside + ``_validate_executable_reference_families``. Two malformed-intent checks, + scoped to the config / parameters path (not a blanket envelope forbid): + + - **Rule A** — a supplied ``data.revision`` must be double-nested + (``{"data": {...}}``). A single-nested revision is dropped silently by + ``resolve_revision`` and would run a seeded default and then fail late; + reject it here with the fix instead. + - **Rule B** — when references are the intended config source (no inline + parameters, no usable revision), they must pin one committed config. A bare + family root (``application`` / ``workflow`` / ``evaluator``) resolves + nothing, so it is rejected in favor of a variant, an environment, or a + revision. + + An empty request (no revision, no references, no inline parameters) is left + untouched: config can still arrive from the running context or a pre-installed + handler, and completion / chat must not regress. Only an EXPRESSED but + unresolvable config intent is rejected. + """ + data = request.data + + # Rule A: a present data.revision must be correctly double-nested. + if data is not None and data.revision: + if not _is_double_nested_revision(data.revision): + _raise_bad_request( + "`data.revision` is nested one level too shallow. Its own fields " + 'must go under a `data` key: `data.revision = {"data": {...}}`. ' + + _INVOKE_CALL_SHAPES + ) + + # Inline parameters or a usable revision already resolve the config; done. + has_inline_parameters = bool(data is not None and data.parameters) + has_usable_revision = bool( + data is not None and _is_double_nested_revision(data.revision) + ) + if has_inline_parameters or has_usable_revision: + return + + # Rule B: references are now the intended config source, so they must resolve. + if request.references and not _has_resolvable_reference(request.references): + _raise_bad_request( + "This invoke supplies a reference that pins no committed config, so it " + "cannot resolve to anything to run. " + _INVOKE_CALL_SHAPES + ) + + def _has_embed_markers(config: Any, _depth: int = 0) -> bool: """Check if a configuration contains any @ag.embed markers. @@ -567,6 +664,12 @@ async def __call__( call_next: Callable[[WorkflowInvokeRequest], Any], ): ctx = RunningContext.get() + + # Fail loud at the boundary when the caller expressed a config intent that + # cannot resolve (a single-nested revision, or a reference that pins no + # committed config), instead of silently running a seeded default. + _validate_resolvable_config(request) + revision = await resolve_revision(request=request) request_has_parameters = bool(request.data and request.data.parameters) diff --git a/sdks/python/oss/tests/pytest/utils/test_resolver_middleware.py b/sdks/python/oss/tests/pytest/utils/test_resolver_middleware.py index d8b7d7e173..064010788e 100644 --- a/sdks/python/oss/tests/pytest/utils/test_resolver_middleware.py +++ b/sdks/python/oss/tests/pytest/utils/test_resolver_middleware.py @@ -554,9 +554,12 @@ async def test_direct_lookup_leaves_selector_unset(self): WorkflowRevisionData, ) + # A variant is a direct (non-environment-backed) lookup, and it is a + # resolvable reference (a bare `application` is now rejected at the + # boundary, so it can no longer stand in for the direct-lookup case). request = WorkflowInvokeRequest( credentials="test-creds", - references={"application": {"slug": "my-app"}}, + references={"application_variant": {"slug": "my-app.default"}}, ) revision = WorkflowRevisionData(uri="test://uri", parameters={"model": "x"}) @@ -629,6 +632,106 @@ def test_ignores_none_reference_values(self): ) +class TestResolverConfigValidation: + """Rules A + B: an invoke must resolve to a caller-intended config. + + Two valid shapes only — inline `data.parameters`, or a revision reference + that pins one committed config (a variant, an environment, or a revision, with + a supplied `data.revision` correctly double-nested). A bare `application` + reference and a single-nested `data.revision` are rejected with a clear 400. + """ + + def _validate(self, **kwargs): + from agenta.sdk.middlewares.running.resolver import ( + _validate_resolvable_config, + ) + from agenta.sdk.models.workflows import WorkflowInvokeRequest + + _validate_resolvable_config(WorkflowInvokeRequest(**kwargs)) + + # ---- valid shapes pass ------------------------------------------------- + + def test_inline_parameters_pass(self): + from agenta.sdk.models.workflows import WorkflowRequestData + + self._validate( + data=WorkflowRequestData(parameters={"agent": {"harness": "pi_core"}}), + ) + + def test_double_nested_revision_passes(self): + from agenta.sdk.models.workflows import WorkflowRequestData + + self._validate( + data=WorkflowRequestData( + revision={"data": {"uri": "test://uri", "parameters": {"m": "x"}}}, + ), + ) + + def test_resolvable_variant_reference_passes(self): + self._validate(references={"application_variant": {"slug": "app.default"}}) + + def test_resolvable_environment_reference_passes(self): + self._validate(references={"environment": {"slug": "production"}}) + + def test_resolvable_revision_reference_passes(self): + self._validate( + references={ + "application_revision": {"slug": "app.default", "version": "3"} + }, + ) + + def test_empty_request_passes(self): + # No expressed config intent — config may arrive from the running context + # or a pre-installed handler (the completion / chat interface-default path + # must not regress). Nothing to reject here. + self._validate() + + # ---- malformed intent rejects ----------------------------------------- + + def test_bare_application_reference_rejects(self): + with pytest.raises(ValueError) as excinfo: + self._validate(references={"application": {"slug": "my-app"}}) + message = str(excinfo.value) + assert "not resolvable" in message + assert "application_variant" in message + assert getattr(excinfo.value, "status_code", None) == 400 + + def test_bare_workflow_reference_rejects(self): + with pytest.raises(ValueError, match="pins no committed config"): + self._validate(references={"workflow": {"slug": "my-wf"}}) + + def test_single_nested_revision_rejects(self): + from agenta.sdk.models.workflows import WorkflowRequestData + + with pytest.raises(ValueError) as excinfo: + self._validate( + data=WorkflowRequestData( + revision={"uri": "test://uri", "parameters": {"m": "x"}}, + ), + ) + message = str(excinfo.value) + assert "one level too shallow" in message + assert '{"data": {...}}' in message + assert getattr(excinfo.value, "status_code", None) == 400 + + @pytest.mark.asyncio + async def test_bare_application_rejects_through_middleware(self): + from agenta.sdk.middlewares.running.resolver import ResolverMiddleware + from agenta.sdk.models.workflows import WorkflowInvokeRequest + + request = WorkflowInvokeRequest( + credentials="test-creds", + references={"application": {"slug": "my-app"}}, + ) + + mw = ResolverMiddleware() + call_next = AsyncMock(return_value="result") + with tracing_context_manager(TracingContext()): + with pytest.raises(ValueError, match="not resolvable"): + await mw(request, call_next) + call_next.assert_not_called() + + class TestResolverMiddlewareTracingParameters: """Tests that ResolverMiddleware mirrors the resolved parameters onto TracingContext, so the root span records them under ag.meta.configuration.