feat(agents): emit a machine-readable approval decision on gated PXI tools - #15029
Merged
Conversation
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
dosubot
Bot
added
the
size:L
ehutt
force-pushed
the
ehutt/pxi-approval-marker
branch
from
August 4, 2026 00:50
13bbdad to
5d538b4
Compare
Contributor
Author
|
Filed #15033 for the known gap called out in the description: Deliberately not fixed here — closing it means setting The gap is linked from |
Collaborator
|
Flagged priority: high — @axiomofjoy you're the assignee on this one, please take a review pass. Raised during standup triage on 2026-08-10. |
axiomofjoy
reviewed
Aug 12, 2026
| _SOURCES = frozenset({"user", "auto"}) | ||
|
|
||
|
|
||
| def approval_attributes(result: Any) -> dict[str, str]: |
Contributor
There was a problem hiding this comment.
let's add a type on this input
axiomofjoy
reviewed
Aug 12, 2026
Contributor
There was a problem hiding this comment.
let's move this into the agents.py router file
axiomofjoy
approved these changes
Aug 12, 2026
Approval-gated PXI tools each hand-rolled their accept/reject output, so the
payload vocabulary had drifted: most emit `status: "accepted" | "rejected"`,
while `save_prompt` demotes the verdict to `approvalStatus` and sets
`status: "saved"`, and `load_dataset` / `patch_experiment` use `"loaded"` /
`"applied"`. Nothing in span metadata marked a call as approval-gated at all —
rejections complete with status OK like any other tool call.
That left trace consumers no way to identify an approval decision except by
matching a hand-maintained list of tool names, which had already gone stale
(every dataset-write tool is gated and was missing from it).
Add a reserved, nested `approval: { decision, source }` marker, stamped by a
shared `approvalOutcome()` helper in every accept/reject path. It is:
- nested, so tools that spread their own action result into the output
(`save_prompt`, `write_prompt_tools`) cannot clobber it;
- additive, leaving existing `status`/`acceptedBy` keys untouched, so the
tool-card UI parses exactly what it parses today;
- meaningful by its absence — cancellations and still-pending proposals stay
unmarked, so consumers can skip them rather than guess.
The server promotes the marker onto the emitted TOOL span as the
`pxi.approval.decision` / `pxi.approval.source` attributes, in the single place
every client-tool result becomes output attributes. This makes approval
decisions filterable server-side via `get_spans(attributes=...)` instead of
requiring consumers to fetch every TOOL span and scan `output.value`, and
centralizes the defensive payload parsing once rather than per consumer.
A source-level drift guard asserts every `pending*.ts` module that emits tool
output stamps both branches, so a newly gated tool cannot silently go unmarked.
…made The first pass set the span attributes in `ToolSpanMixin.set_output`, which approval-gated tools never reach: they are external tools, so they never execute server-side (`execute_tool_call` raises before the hooks run, and resumption assigns the browser's result directly). Their TOOL spans are synthesized from the request body in `_synthesize_client_tool_spans`, which is the only seam where the marker can reach the span — verified against real spans, whose `session.id`/`tool.id` attributes only that path emits. Move the extractor to `phoenix.server.agents.approval`, call it where the output attributes are actually built, and revert the mixin. Add a router test asserting the attributes land on a synthesized gated span and stay absent on a non-gated one — the test that would have caught this. Also from review: - Guard the extractor's frozenset membership tests with isinstance checks. The marker is browser-supplied JSON, so an unhashable `decision` raised TypeError out of span emission, contradicting the "never fails a tool call" contract. - Stamp `source: "auto"` as a literal in the evaluator auto-accept branch rather than echoing `result.acceptedBy`, which a future unwrapped submit host could set to "user". - Find approval payloads by content rather than by `pending*.ts` filename. The old guard missed `agent/tools/approval.ts` — a payload site this very change had to patch — and now pins the full emitter list. - Document the pre-existing `submit_*_evaluator_draft` gap: those decisions happen in a dialog that never writes tool output, so they stay invisible.
The gap is documented where a reader of the marker would hit it; point it at issue #15033 so the follow-up is findable from the code rather than only from the tracker.
ehutt
force-pushed
the
ehutt/pxi-approval-marker
branch
from
August 12, 2026 22:11
3e935df to
8d56d0b
Compare
Contributor
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
ehutt
enabled auto-merge (squash)
August 12, 2026 22:22
…ns (#15030) * feat(evals): add suggestion_accepted online eval targeting approval-gated TOOL spans * test(evals): cover suggestion_accepted semantics and mixed root/TOOL runner targeting * test(evals): add sanitized real-shape suggestion approval trace fixtures * docs(evals): document suggestion_accepted target discovery and approval semantics * fix(evals): enforce span selector parent matching * refactor(evals): discover suggestion outcomes by approval attributes `suggestion_accepted` identified approval decisions by matching `tool.name` against a hand-maintained `APPROVAL_GATED_TOOLS` list, then parsing `output.value` through a defensive multi-shape decoder because each tool spells its outcome differently (`accepted`, `saved`, `loaded`, `applied`, `removed`, and `save_prompt`'s `approvalStatus`). That list was already stale before shipping: every dataset-write tool is approval-gated via `stageDatasetWrite` and none were listed, so those decisions went unmeasured. Keeping it correct meant maintaining a cross-language contract with the frontend by hand, with nothing enforcing it. PXI tools now stamp a uniform approval marker that the server promotes onto the span, so: - Delete the allowlist and the name-based applicability gate. Discovery selects on `pxi.approval.source = "user"` — one server-side query that yields exactly the annotated set, since rejections are always a user action and automatic accepts are never annotated. - Delete the `output.value` decoder. Classification reads `pxi.approval.decision` only, so a tool's own status vocabulary is irrelevant and a look-alike payload on a non-gated tool can no longer be misread. - Extend `SpanSelector` with hashable attribute filters, letting an evaluator target spans by what they record rather than by which tool produced them. A selector still requires a name or attribute filter so discovery stays bounded. A newly approval-gated tool is now measured the day it ships. Fixtures keep each tool's original status vocabulary alongside the marker, which is what makes them worth keeping: they prove classification no longer depends on it. * fix(evals): harden approval-attribute discovery after review - Import the attribute names from `phoenix.server.agents.approval` rather than re-declaring the literals. The eval reads what the server writes, and a drifted name would not raise: discovery would return nothing, forever, and look like a quiet window. A test pins the two modules together. - Isolate per-selector discovery failures. Attribute filtering requires a newer Phoenix server than name filtering, so an old server or a transient error on one selector previously aborted the whole scheduled job, taking `tool_count_per_turn` and `user_friction` with it. The candidate-limit guard stays fatal via its own `CandidateLimitError`: unlike a failed query, a truncated candidate set makes the run's results quietly incomplete. - Reject non-string attribute values in `SpanSelector`, which would otherwise serialize into the query and then never match locally. - Apply the attribute filter in the runner's fake `get_spans`, so a test would notice if server-side filtering silently stopped working. - Docs: soften the drift-guard claim (it recognizes payloads by the known accept/reject vocabulary, so a hand-rolled tool inventing new wording is not covered — tools on `bindPendingApproval` are covered by construction), and state that pre-marker spans are invisible to discovery and cannot be backfilled. * docs(evals): link the unmeasured submit_* tools to their tracking issue Note in both eval docs that accept/reject rates exclude the two submit tools until #15033 lands, so the gap is visible to anyone reading the numbers. * refactor(evals): simplify suggestion outcome eval * style(evals): format rebased online evals * fix(evals): type suggestion fixture payload
dosubot
Bot
added
size:XL
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Approval-gated PXI tools each hand-roll their accept/reject tool output, and the vocabulary has drifted:
status: "accepted",acceptedBysave_promptstatus: "saved", verdict demoted toapprovalStatusload_datasetstatus: "loaded"patch_experimentstatus: "applied"remove_prompt_instancestatus: "removed"Nothing in span metadata marks a call as approval-gated at all — a rejection completes with status
OKlike any other tool call.So a trace consumer has no way to identify an approval decision except by matching a hand-maintained list of tool names. We were about to merge exactly that: the
suggestion_acceptedonline eval carries anAPPROVAL_GATED_TOOLSallowlist, and it had already gone stale — every dataset-write tool is approval-gated viastageDatasetWriteand was missing, so those user decisions went unmeasured.What
A reserved, nested marker stamped by a shared
approvalOutcome()helper in every accept/reject path:save_promptandwrite_prompt_toolsspread their own action result into the output and would clobber any top-level key. The marker is spread last everywhere; a test pins that ordering.status/acceptedBykeys are untouched, so the tool-card UI parses exactly what it parses today. Every output parser is a non-strict zod object that strips unknown keys; no UI change.Promoted to span attributes
pxi.approval.decision/pxi.approval.sourcein_synthesize_client_tool_spans, where client-tool spans are actually built. This lets consumers filter decisions server-side viaget_spans(attributes=...)instead of fetching every TOOL span and scanningoutput.value, moves the defensive payload parsing server-side once rather than per consumer, and makes decisions visible in the Phoenix UI. Extraction never raises — the payload is browser-supplied, so malformed markers are ignored rather than failing a tool call.A drift guard finds approval payloads by content and asserts each stamps the marker, so a newly gated tool can't silently go unmarked. That's the whole point: consumers should never need the tool-name list again.
Notes for review
ToolSpanMixin.set_output; that was wrong and is fixed in the second commit. Gated tools are external tools that never execute server-side, so their spans are synthesized from the request body instead. Worth a look at the router test that pins this.submit_code_evaluator_draft/submit_llm_evaluator_draftresolve asawaiting_userand the user's real decision happens in a dialog that never writes tool output, so those decisions stay invisible in traces.bindPendingApprovalcore — each carries bespoke logic (edit summaries, revision staleness checks, cancel paths) and some of these tools may be short-lived.Testing
pnpm vitest run src/agent— 727 passedpytest tests/unit/server/agents/— 295 passedpnpm typecheck,mypy,make lint-python,make lint-frontend— cleanStacked on this
suggestion_acceptedonline eval (follow-up PR) deletes itsAPPROVAL_GATED_TOOLSallowlist and discovers spans by thepxi.approval.*attributes instead.