feat(sdk): add annotate_trace platform-op#4999
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Adversarial review of
|
|
Design-doc-only PR (the implementation code is being stripped; this is the Feedback I need before implementing:
Approve the design (or answer the three) and I'll implement. |
2f12208 to
dd336df
Compare
dd336df to
9d67891
Compare
|
|
||
| - Reserve a single evaluator. Proposed slug: `agent-self-reflection` (final naming is open, | ||
| see status.md). | ||
| - Give it a **permissive** outputs schema so freeform reflection always validates and the shape |
There was a problem hiding this comment.
First, check whether something like this is allowed to have something like a really object without any additional property. I think it's preferable to have a simple schema and then have some kind of modal or additional stuff where it's an object when you can add stuff. Basically, you have a string with reflection and then something like a score or judgment, which is binary, like good or bad, so that you can filter with success or not, successful or not. In addition to that, you would add something additional, kind of meta quirks or whatever, that could be an object. In this way, you'd expect the reflection to be more structured. Also, we can't show it in the UI, because otherwise it would become blending in both senses.
This also means that we need to add this kind of evaluator by default when creating a project or whatever, and probably also add a migration for it, the same way we do with our evaluator quality rating.
|
|
||
| This is an implementation choice, resolved at build time, not by the op contract: | ||
|
|
||
| 1. **Seed it per project**, exactly like `quality-rating`: add a preset on the |
| `agenta:custom:feedback:v0` catalog entry plus an entry in `_DEFAULT_EVALUATORS`. New | ||
| projects get it for free. Existing projects need a one-time backfill migration (the same | ||
| shape that backfilled `exact-match` / `contains-json`). | ||
| 2. **Ensure-exists at annotation time**: when an annotation targets the reserved slug and no |
There was a problem hiding this comment.
Yeah, we should not do this, but we should add it maybe to the skill as a kind of resource for fallbacks, like troubleshooting. If you get an error that the evaluator does not exist, then create an evaluator using this, and here's the scheme of the evaluator you need to create, something like that.
| code-defined reserved evaluator (mirroring the reserved `_agenta.*` platform-skills pattern) is | ||
| a later consolidation if reserved artifacts multiply. | ||
|
|
||
| ## 2. The self-targeting guarantee |
There was a problem hiding this comment.
I don't understand this section. Please rewrite it with more context. What do you mean by an invariant and the server-owned annotation links? Why are we talking about this? Is it how the request should be or what?
| } | ||
| ``` | ||
|
|
||
| The model never sees `references` or `links`. |
|
|
||
| ## Permission | ||
|
|
||
| Default `allow` / no approval (`default_permission="allow"`, |
| approval on every self-reflection would defeat the autonomous use case. Authors can still | ||
| override per config (`permission` / `needs_approval` on `PlatformToolConfig`). | ||
|
|
||
| One residual risk: each call still writes an annotation trace, so a runaway loop could spam |
There was a problem hiding this comment.
I agree, it's not a problem.
mmabrouk
left a comment
There was a problem hiding this comment.
Update according to the review. Please add inline comments for your updates and answer my comments, and then have a high-level comment saying how you updated this PR and if there are any open questions.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3020a440-9a12-4503-8df9-65b344adaf15
📒 Files selected for processing (5)
docs/design/agent-workflows/projects/builder-agent-reliability/annotate-op/README.mddocs/design/agent-workflows/projects/builder-agent-reliability/annotate-op/context.mddocs/design/agent-workflows/projects/builder-agent-reliability/annotate-op/plan.mddocs/design/agent-workflows/projects/builder-agent-reliability/annotate-op/research.mddocs/design/agent-workflows/projects/builder-agent-reliability/annotate-op/status.md
| Default `allow` / no approval (`default_permission="allow"`, | ||
| `default_needs_approval=False`). | ||
|
|
||
| With a reserved, pre-existing evaluator there is no evaluator-creation side effect per call. | ||
| The op writes additive self-metadata onto the agent's own trace and nothing else. Requiring | ||
| approval on every self-reflection would defeat the autonomous use case. Authors can still | ||
| override per config (`permission` / `needs_approval` on `PlatformToolConfig`). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Don't default to allow until the enforcement path exists.
This choice depends on the runner-side subtree replacement / arg stripping work later in the plan. If the op becomes callable before that lands, the self-targeting gap reopens.
Suggested edit
- Default `allow` / no approval (`default_permission="allow"`,
- `default_needs_approval=False`).
+ Default `ask` until the runner-side enforcement lands (`default_permission="ask"`,
+ `default_needs_approval=True`).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Default `allow` / no approval (`default_permission="allow"`, | |
| `default_needs_approval=False`). | |
| With a reserved, pre-existing evaluator there is no evaluator-creation side effect per call. | |
| The op writes additive self-metadata onto the agent's own trace and nothing else. Requiring | |
| approval on every self-reflection would defeat the autonomous use case. Authors can still | |
| override per config (`permission` / `needs_approval` on `PlatformToolConfig`). | |
| Default `ask` until the runner-side enforcement lands (`default_permission="ask"`, | |
| `default_needs_approval=True`). | |
| With a reserved, pre-existing evaluator there is no evaluator-creation side effect per call. | |
| The op writes additive self-metadata onto the agent's own trace and nothing else. Requiring | |
| approval on every self-reflection would defeat the autonomous use case. Authors can still | |
| override per config (`permission` / `needs_approval` on `PlatformToolConfig`). |
9d67891 to
4897638
Compare
Second-round revision (design docs only, still draft)Addressed the inline review comments. All five workspace files updated; no code. 1. Evaluator schema is now structured, not permissive. 2. Seed by default plus a backfill migration, mirroring 3. No auto-create at annotation time. Dropped the ensure-exists and virtual-evaluator 4. Rewrote the self-target section in plain language. No more "invariant" jargon. It now 5. One open question left: upsert vs append for repeated calls on the same trace. Lean is Feedback wanted: (a) the structured schema in plan.md §1, especially |
4897638 to
872a1b4
Compare
|
Rebased onto big-agents |
Rebased onto big-agents f8765a9. Claude-Session: https://claude.ai/code/session_01WSp2LqKrEtXnm2fsPWuQWa
872a1b4 to
d3eef05
Compare
Adds the annotate_trace op to the platform op catalog: self-targeting (trace/span bound from run context, links hidden from the model), additive self-metadata so it defaults to auto-allow. Tests pin the self-targeting binding. Claude-Session: https://claude.ai/code/session_01DGj7GKafjkZeQXMsryWhb2
What
Adds an
annotate_traceentry toPLATFORM_OPSso a running agent can record an annotation (evaluation feedback) on its own current run's trace — "grade myself". This closes the last gap for the self-reflecting-agent use case (builder-agent-reliability use case 3): the annotations REST API and the run-context trace plumbing already existed; only the catalog entry was missing.How it mirrors
commit_revisionannotate_traceis a mutating, self-targeting platform-op built exactly likecommit_revision:POST /api/annotations/(args_into="annotation", request wrapper is{"annotation": AnnotationCreate}).trace_id/span_idare bound from run context and hidden from the model, so the agent can only ever annotate its OWN trace:commit_revisionbinds$ctx.workflow.variant.idthe same way.)additionalProperties: False) and exposes onlyreferences.evaluator.slug+data.outputs.linksis never model-supplied.Assembled body:
{ "annotation": { "references": { "evaluator": { "slug": "self_reflection" } }, "data": { "outputs": { "score": 0.9, "note": "…" } }, "links": { "invocation": { "trace_id": "<run's own>", "span_id": "<run's own>" } } } }Permission default
Leans
allow/ no-approval — annotating your own trace is additive self-metadata (unlikecommit_revision, which mutates config and defaults toask). Authors can override per-config, andaskis the documented alternative (a brand-new evaluator slug auto-creates an evaluator artifact).Files
sdks/python/agenta/sdk/agents/platform/op_catalog.py— the op + its description/schema constants.sdks/python/oss/tests/pytest/unit/agents/platform/test_op_catalog.py— added to the catalog set + two focused tests (self-target bind + hiddenlinks; auto-allow default).docs/design/agent-workflows/projects/builder-agent-reliability/annotate-op/README.md— design doc.The build-kit overlay enumerates
PLATFORM_OPSdynamically, so the op is offered to builder agents automatically.Verification
pytest oss/tests/pytest/unit/agents/platform/— 87 passed.test_build_kit_overlay.py— 4 passed.ruff format+ruff check --fixclean.Review asks
$ctx.trace.*token names ($ctx.trace.trace_id/.span_id) — the load-bearing binding. Verified againstRunContextTrace,RunContext.to_wire(), andresolveCtxTokenin the TS runner.{"annotation": {data.outputs, references.evaluator, links.invocation.{trace_id,span_id}}}) — verified against the router request model and the annotations acceptance test.allowpermission default vsask.Draft — do not merge.
https://claude.ai/code/session_01WSp2LqKrEtXnm2fsPWuQWa