feat(api): agent behavior leaves the public API: handler-mode ops, one error envelope, routes deleted - #5797
Merged
mmabrouk merged 31 commits intoAug 7, 2026
Conversation
… lands in AGENTS.md; every stack-added error retrofits to {code,message,retryable,next_step?,details?}; the reason wrappers are gone; commit_failed and the ReadConfig family stop claiming retryable on deterministic failures (RevisionConflict too: an unchanged replay carries the same stale base); non_embeddable_reference joins the platform's 400 mapping
…eads as an ERROR. Affects every gateway/Composio tool failure on every harness, plus the handler-mode platform ops the API migration adds. callAgentaTool THROWS instead of returning, so the relay writes ok:false and the MCP shim renders isError true; the model keeps the full text. status.code alone decides; status.message is no longer required for a failure to be noticed (the absent-message arm fell through to plain success before). Known live consequence: test_run's infrastructure arm now surfaces as a failure carrying its response instead of as a success, pending the infra-vs-domain contract answer.
…stops assuming TestRunResponse; STATUS_CODE_ERROR on /tools/call ALWAYS carries the canonical envelope (test_run's infra arm adopts it as test_run_incomplete, retryable true, trace_id in details; infra_failure deleted with its router coupling); nothing the API emits deliberately can hit the runner's non-envelope fail-safe
…very. Audited code by code: only source_not_found survives as retryable (the agent writes the missing file and the SAME request succeeds; commit_lock_timeout on the router is its twin). Eight formerly-terminal codes gained the next_step they lacked, with a test asserting no code is a dead end. The contract's 12.1/12.2 become Correctable vs Terminal refusals, and the commit tool's description tells the model to read next_step regardless of retryable.
…e: platform handlers always emit it; the router's gateway and workflow-tool arms pass their own shapes through (delivered as errors since the contract fix), so the non-envelope pass-through is load-bearing, not dead code
… justifies: all three router producers serialize ONE content expression branching only on status.code; only the handler seam varies content, inside the handler, by design; a new producer differing across branches is the review flag
…andler-mode ops on /tools/call. Permissions TIGHTEN (unconditionally elevated VIEW/EDIT_WORKFLOWS on top of RUN_TOOLS; an empty payload cannot read as no-elevation, two cells force it); the variant binding fails closed in four shapes; scope stays engine-enforced with the reasoning in the docstring; every expected failure returns the envelope. Adds the destination without removing the origin: the routes still exist until step 9, the catalog still points at them until step 7.
…atform handlers emit it; gateway and workflow-tool arms carry their upstream's shape symmetrically; do not delete a consumer's non-envelope path on the unconditional reading); open-issues gains the no-next_step-on-gateway-failures candidate, decided by the v2 benchmark's numbers
…ig call ref (a pure transport swap, live-proven on the preview stack in three arms incl. a forced domain failure whose envelope carried the children list as free recovery); the previously-uncovered helper gains 7 direct tests; both wire shape tests unskip; no skips remain in the suite
…marker cap (readability) is removed WITH the card collapsing change or after it, never before; the 32-entry turn store bound is a separate MEMORY decision, not inherited from the readability argument
…s on TOOL PRESENCE (renders only when commit_revision is in the spec set; the mount paragraph is deliberately not gated; the flag is recorded as the wrong axis); the two bare approval-plumbing strings gain the module's what-happened/no-retry/what-next shape without claiming a person or policy refused; plus the step-8 straggler docstring in direct.ts
… site (second pass; hunk attribution split it from the batch)
…-live switch Mahmoud asked for: both routing halves derive from it so the two known-broken half-flipped states are unrepresentable; unknown harnesses stay fail-closed regardless; six tests incl. the atomicity property and a blast-radius check; the constant's comment records that a green unit suite after a flip proves wiring only, and L5 is the acceptance test for believing it
… (symptom, mechanism, cost both ways, acceptance test, seven of each); two live findings that existed only in session messages are written down for the first time (pre-gate marker validation; the client-tool row recorded cancelled after fulfilment, runner half code-confirmed); refresh-then-reopen leads with FIXED status and names the switch as its activation mechanism
…ns: the rendered file is a copy (edits are overwritten and invisible to the user) AND may be stale (a change after run start may not appear), ending on the positive instruction naming read_config and commit_revision; 'may not appear' is deliberate because a request carrying changed instructions IS re-rendered (L5 proves it live) and a test pins the absolute form out; open-issues records the follows-the-request-not-the-stored-revision decision with the draft-run argument that settles it and fm-02 zero-of-nine as the revisit trigger
…ed cold: 19 wire-recorded occurrences, 16 the same XML parameter-tag leak, the single-visible-property hypothesis marked untested, why no shape fix ships this release (tool structure frozen by ruling; the failure costs a retry, not a wrong result), and the acceptance test that proves any future fix
…est pointer: a second harmless visible property on target in a throwaway catalog build, one benchmark run, confirms or kills the hypothesis without touching shipped tool structure
…ding 1: pre-writing the version-hash marker is marked UNTESTED (a fresh CODEX_HOME stays empty through codex --version, so materialization is session-start; nothing verified says a pre-written marker suppresses it); findings 2 and 3 stand on directly verified evidence and the refusal to delete rests on them
…_context bool (default False) threaded through the checked commit service; the commit handler and test_run's preview opt in, every other caller (playground save, SDK, applications, evaluators) takes the general path. Scope ENFORCEMENT deliberately not gated and pinned by a cell that refuses the same out-of-scope operation in both contexts (confinement stays a property of the entry point that hands out the policy, not of a boolean a caller could be handed wrongly). C24/C25/C26/C43 each proven closed by running the SAME payload both ways: caller message survives generally and is derived for the agent; a platform tool in a human's tools list commits generally and is refused for the agent; the repeated-list-name selector is corrected-with-warning for the agent and precisely refused for a program; the legacy arm takes the general path unless the caller is the agent. Plus a cell pinning that the preview runs in the agent's context so preview and commit cannot normalize differently
…nce_from_data and its full-data call are gone; the general path never silently mutates caller data again; the engine-side strip on operation values stays because it serves the agent arm) and C38 closed by RELOCATION (the core read no longer takes run_is_draft at all; the handler owns the draft fact as a server-side context binding like the variant id, so an agent cannot assert draftness to change the answer it gets). The reopened surface is deliberate and pinned: a human pasting a rendered file into the playground stores the guidance block as their own text, because an agent cannot reach the full-data path at all (its handler refuses the shape with full_data_not_committable); both halves are stored-row cells against a real database with the reasoning in the class docstring. Deletion proved to change behavior on the same payload before any suite was read
…t re-implemented. PlatformHandlerResult gains committed_revision (plain ids, so the tools layer keeps not depending on the workflows one); the boundary reads it and calls invalidate_cache plus the emitter already in the tools router. No-event-on-no_change is proven at BOTH layers: handler cells pin when the field is set, and two cells drive the REAL router asserting the effects themselves (a commit invalidates and emits; no_change does neither), because a perfect handler feeding an ignoring boundary would pass every handler cell and still leave warm sessions stale. One cell targets the invisible-by-construction failure: the emitter returns early on any missing key, so key-name drift would silently kill the event under a green suite; that cell feeds the real emitter and asserts something came out. The no_change docstring records why that half is the feature: an event there would throw away the warm session the no-change answer exists to keep
…: milestone 5 removed run_is_draft from the core read while the route still passed it, so the live endpoint 500ed on every call (TypeError at resolution; caught by the benchmark's direct no-model probe bracketing the landing window). The route now decorates its own answer exactly as the handler does (draft warning appended route-side from the context-bound field), with the comment recording why the core stays ignorant of who is asking. The route remains load-bearing until the catalog flip reaches deployed runners, and it is scheduled for deletion in milestone 9
…ps carry handler call_refs and no method/path; the allowlist gains both; read_config keeps read_only and its 15s timeout; context bindings ride as spec-level bindings the relay injects and fail CLOSED when missing (a missing binding raises and stays a non-2xx the runner redacts, because it is runner-caused, not model-caused). The kill switch fails LOUDLY for the two config ops (GatewayToolResolutionError naming the variable, the capability removed, and the one-step fix; exercised live, not asserted) while test_run keeps its quiet skip, with the asymmetry reasoning in the code: skipping an optional op is a degradation, skipping the only transport for a core capability is an outage wearing a warning's clothes. Model-authored bad arguments come back as invalid_arguments envelopes with next_step over HTTP 200, three malformation shapes covered. Option 3 (gating commit_revision's tool-list membership on the ordered-operations flag) is recorded in open-issues as Mahmoud's decision with the three-way analysis; the read_config kill-switch cell's skip condition documents WHY the two ops differ in both states, which is the fact that decides the upgrade blast radius
…t reads each route call site and asserts every keyword it passes is one the REAL service class accepts (no mocks, no database), for both the read and the commit. Deliberately about AGREEMENT rather than behavior, because behavior is what every mocked route test already covers and agreement is exactly what none of them could see (2133 green while every live call 500ed). Bite-proven by reintroducing the exact bug: 2 of 3 cells fail. The docstring names the incident so nobody replaces this with something that mocks the service and feels more normal
…allers moved first: the gate helpers went to /tools/call before the deletion so nothing pointed at a stack lacking the routes even between hot reloads (the read-config outage's lesson applied forward). Live-verified: OpenAPI contains neither path while the general commit route and /tools/call remain; commit/agent answers 404; read-config answers 405 rather than 404 because the {workflow_revision_id} GET route now matches that path segment, so a stale POST is method-rejected, still loud; a missing variant binding on /tools/call returns 400, milestone 7's fail-closed binding working live. The seam test keeps its COMMIT cell deliberately (that route survives for humans and the SDK, so its seam still needs the guard that the read route's absence made moot); the four scoped-agent-route cells are deleted with a pointer to the handler-boundary tests that hold the same guarantees
… switch (documented default ON, both halves shipped, disabling fails loudly for the two config ops) and its catalog rows point at the handler call_refs; the read-config contract gains section 16 recording the route deletion, why (every detail agent-shaped, no second consumer), and the two invariants that did NOT move (unforgeable confinement; scope enforcement in the engine); two stale spots inside the contract fixed (the catalog snippet showing method=POST beside the handler, and a paragraph claiming the op needs a new endpoint); the duplicate section number the append created is renumbered
…tion race (132 evictions, 63 ACP write retries, 87 percent adjacency, 42 percent hit rate; two candidate fixes with their trade; the acceptance test must be concurrent BY CONSTRUCTION because a serial run passes against the broken code, and every existing gate cell is serial, which is why nothing caught it; a turn that stops talking mid-retry is the user-facing symptom; a log that starts at T cannot testify about T minus one) and the rebuild-reproducibility finding in its corrected form (the floating base tag is a real hazard, but that night's build used a three-week-old cached base with pull disabled, so Node/undici were byte-identical and fd was the only variable; the accident of the cache ends silently with --pull, a prune, or CI)
…pe. The description's examples were bare path arrays with no key and no wrapper, so models copied exactly what they saw and each harness fumbled the missing envelope in its own dialect: pi sent target as the bare array (rejected 'must be object', 32 times in one pre-milestone-7 cell, a 48-point one-shot gap), claude reached for an XML parameter tag to NAME the wrapper it had nowhere to copy (16 malformed-JSON failures). Both examples are now complete argument objects and the prose states the nesting as structure rather than a dotted path. No schema change: the schema was always right, only the description misrepresented it. Benchmark evidence and prediction (collapse recovered and the malformed-argument class; wrong_surface and the fm-01 control must not move) recorded in benchmarks/agent-config-editing
dosubot
Bot
added
the
size:XXL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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:
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 |
…vidence: status corrected up front (real in runner logs, recovering on retry, never yet shown to reach a user; the observable symptom is NONE, with the eight-file wire grep as evidence); the provider discriminator recorded as decisive because a transport fault cannot be selective by model; the two-layers-same-number trap named (client text 'Retrying (attempt 1/3)' and runner log 'ACP write retry 1/3' are different mechanisms wearing the same number, which is why three people converged on the wrong attribution); surfacing ACP failures onto the wire promoted to fix (1) because without it the other fixes have no external signal to evaluate; the concurrent-by-construction acceptance test now asserts on wire-visible evidence and is explicitly unwritable until fix (1) exists
…ming the layer and endpoint in the runner's ACP retry line and not formatting it as N/3 costs one string and removes the misattribution class that consumed most of a night; explicitly worth doing with the next runner touch and NOT worth a landing of its own
Contributor
Railway Preview Environment
|
mmabrouk
changed the base branch from
agent-config-editing-s9-steer-mount
to
release/v0.110.0
August 7, 2026 10:18
mmabrouk
added a commit
that referenced
this pull request
Aug 7, 2026
…migration feat(api): agent behavior leaves the public API: handler-mode ops, one error envelope, routes deleted
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
1 participant
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.
Context
Mahmoud's API review found that the agent-config-editing stack had put agent-specific behavior on the public API surface: two endpoints whose entire contract serves one consumer (
POST /workflows/revisions/read-configandPOST /workflows/revisions/commit/agent), plus several one-off error shapes. This PR executes the approved migration plan: both endpoints are deleted, their behavior moves behind handler-mode platform ops dispatched through the existing genericPOST /tools/call, and every expected domain failure an agent can see now uses one envelope.Changes
One error envelope, written down as a rule.
{code, message, retryable, next_step?, details?}, with the rule text inapi/AGENTS.md. The key semantic fix:retryablenow means only "replaying the unchanged request may succeed" (previously it was conflated with "correctable");next_stepis the one-step recovery a small model can follow. Every error this stack added is retrofitted onto the shape.Both ops are handler-mode. The catalog entries carry handler call_refs and no route; the handlers call the workflows core in-process. Permissions elevate RUN_TOOLS with VIEW_WORKFLOWS (read) and EDIT_WORKFLOWS (commit), all three combinations tested. Context bindings (
workflow_variant_id,run_is_draft) are server-bound and fail closed: a missing binding is a non-2xx runner-context refusal, never a half-bound call. Model-authored bad arguments come back asinvalid_argumentsenvelopes over HTTP 200 so the model can actually read them (the runner hides non-2xx tool bodies by design).Agent behavior left the general path. One
agent_context: boolon the checked commit service (default off) carries the agent-only transformations: selector normalization with its warning, platform-tool rejection, and the derived commit message. The playground save, SDK, applications, and evaluators take the general path again. Scope enforcement deliberately stays in the engine, ungated, pinned by a test that refuses the same out-of-scope operation in both contexts.run_is_draftleft the public read surface entirely and became a context binding, so a caller can no longer assert draftness about itself. The full-data guidance strip is deleted outright: an agent cannot send a full-data commit at all, and the general path must never silently mutate caller data (a human pasting a rendered file now stores their own text; both halves pinned as stored-row tests).Side effects moved with the commit. The warm-session cache invalidation and the
committed-revisionfrontend event now hang off the handler result at the tools boundary, reusing the existing emitter. No-event-on-no_change is proven at both the handler and the real router, because an event on no_change would throw away the warm session the no-change answer exists to keep.Routes deleted, callers first. The gate helpers moved to
/tools/callbefore the deletion so nothing was ever stranded on a hot-reloading stack. The live OpenAPI contains neither path. A stalecommit/agentPOST answers 404; a staleread-configPOST answers 405 (the{workflow_revision_id}GET route matches that path segment), still loud. Both generated clients are regenerated on the clients lane (#5785): exactly the deletions, break-tested (tsc, compileall + import, consumer grep).Kill switch and upgrade note
AGENTA_AGENT_ENABLE_PLATFORM_HANDLERS(default on) previously only skippedtest_run. After this PR, a deployment that explicitly set it off fails agent runs at tool resolution with an error naming the variable, the removed capability, and the one-step fix. That is deliberate: silently dropping the agent's only config transport would make the model improvise (write workspace files and report success).test_runkeeps its quiet skip. The affected population is operators who found an internal, design-doc-only variable and disabled it; plausibly zero.One open decision is recorded in
docs/design/agent-config-editing/open-issues.md:commit_revisionsits in every build-kit agent's tool list unconditionally, whileread_configis gated by the ordered-operations flag. Gating both on the flag would make the flag mean what it appears to mean, but that changes flag-off behavior and is Mahmoud's call. This PR is compatible with any answer.Why the wire-level and stored-row tests exist
Five times during this work, every component was green while the composition was dead: a vault test fixture that could not bite, a grep that dropped FAILED lines, an emitter that key-name drift would silently kill, a route whose tests all mock the service (the suite was green while the live route 500ed for 28 minutes), and a str.replace that matched nothing. The seam tests in this PR (route-service call-site agreement, the real-router event cells, the live proof cells) each target a seam where two sides must agree, because each side's opinion of itself was demonstrated worthless five times in one night.
Tests
/tools/callexclusively.What to QA