-
Notifications
You must be signed in to change notification settings - Fork 606
[#5162] fix(frontend): forward variant references on draft playground runs #5163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
35 changes: 35 additions & 0 deletions
35
docs/design/agent-workflows/projects/draft-run-references/README.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| # Draft-run references | ||
|
|
||
| An agent that edits its own configuration can commit once, then every later commit in the | ||
| same chat fails with `missing run-context value for direct-call binding | ||
| 'workflow_revision.workflow_variant_id'`. The page has to reload to recover. This workspace | ||
| explains why and proposes the fix. | ||
|
|
||
| The cause: on a run with unsaved config edits, the playground sends `references: null`, which | ||
| drops the variant identity that the `commit_revision` tool needs. A successful commit is what | ||
| flips the panel into the unsaved state, so the agent's own success breaks its next commit. | ||
|
|
||
| The recommended fix: keep dropping the committed-revision reference on a dirty run (that is | ||
| the correct draft signal), but keep forwarding the variant reference, because the variant is | ||
| what `commit_revision` targets. Variant identity and draft-ness are independent. | ||
|
|
||
| ## Files | ||
|
|
||
| - [context.md](context.md): what the user sees, why it matters, goals and non-goals, and the | ||
| three layers involved. | ||
| - [research.md](research.md): the full trace of the bug through the frontend, the SDK and | ||
| service, and the runner. Includes the worked before/after reference blocks and the verified | ||
| proof that the frontend fix is safe. Read this to understand the mechanism. | ||
| - [plan.md](plan.md): the two fix options, their trade-offs, the recommendation, the | ||
| draft-mode interaction, the acceptance checks, and the test plan. | ||
| - [status.md](status.md): current state, decisions, verified citations, provenance, and | ||
| recorded follow-ups. | ||
|
|
||
| ## Start here | ||
|
|
||
| Read context.md, then research.md (it proves the mechanism before anything is proposed), then | ||
| plan.md. | ||
|
|
||
| ## Tracking | ||
|
|
||
| Issue: https://github.com/Agenta-AI/agenta/issues/5162 |
71 changes: 71 additions & 0 deletions
71
docs/design/agent-workflows/projects/draft-run-references/context.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| # Context | ||
|
|
||
| ## The problem in one sentence | ||
|
|
||
| An agent that edits its own configuration can commit the change once, then every later | ||
| commit in the same chat fails with a run-context error, until the page reloads. | ||
|
|
||
| ## What the user sees | ||
|
|
||
| You open an agent in the playground and chat with it. You ask it to add some tools. The | ||
| agent calls the `commit_revision` tool to save the new configuration. The first commit | ||
| works. A new version appears. | ||
|
|
||
| Then you ask for another change in the same conversation. The agent calls `commit_revision` | ||
| again. This time it fails. The error is: | ||
|
|
||
| ``` | ||
| missing run-context value for direct-call binding 'workflow_revision.workflow_variant_id' | ||
| ``` | ||
|
|
||
| Every commit after the first one fails the same way. The only fix is to reload the page. | ||
|
|
||
| This was reproduced in two recorded sessions: | ||
|
|
||
| - `c6de1865` on 2026-07-07. | ||
| - `8110bba2` on 2026-07-08. | ||
|
|
||
| Both timelines are saved at the repository root as `turn-*.md` files. | ||
|
|
||
| ## Why this matters | ||
|
|
||
| Self-editing is the core loop of the builder agent. The whole point is that you talk to | ||
| the agent and it configures itself: it adds tools, edits its prompt, changes its model, | ||
| then commits the result. If the second commit in a conversation always fails, the agent | ||
| can make exactly one change per page load. That breaks the core experience. | ||
|
|
||
| ## What this project delivers | ||
|
|
||
| This is a design-only workspace. It explains the bug from the ground up, lays out two fixes | ||
| with their trade-offs, recommends one, and defines how to verify it. No code changes here. | ||
| Implementation follows after review. | ||
|
|
||
| ## Goals | ||
|
|
||
| - Explain, for a reader who has never seen this code, exactly how a commit fails. | ||
| - Recommend one fix with clear reasoning. | ||
| - Make sure the fix does not break the draft-mode rules that the same code path relies on. | ||
| - Define acceptance checks and a test plan. | ||
|
|
||
| ## Non-goals | ||
|
|
||
| - Changing the wire contract between the frontend, the SDK, and the runner. | ||
| - Redesigning how run context is assembled. That is a larger effort tracked separately (see | ||
| the related work in plan.md). | ||
| - Fixing the case of a brand-new agent that has never been committed. That agent has no | ||
| variant yet, so its first commit is a different flow. This project targets the reported | ||
| loop: an agent that was committed once and then keeps failing. | ||
|
|
||
| ## Background: the three moving parts | ||
|
|
||
| Three layers cooperate on every agent run. You need all three to understand the bug. | ||
|
|
||
| 1. **The playground (frontend).** It builds the `/invoke` request. It decides what identity | ||
| information to attach to the run, in a block called `references`. | ||
| 2. **The SDK and service (Python).** They receive the request, assemble a "run context" from | ||
| the references, and hand the run to the runner. | ||
| 3. **The runner (TypeScript).** It runs the agent loop. When the agent calls a platform tool | ||
| like `commit_revision`, the runner fills in server-owned fields from the run context. | ||
|
|
||
| The bug is a disagreement between these layers about one field: which variant is running. | ||
| research.md traces the field through all three layers. plan.md proposes the fix. |
212 changes: 212 additions & 0 deletions
212
docs/design/agent-workflows/projects/draft-run-references/plan.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,212 @@ | ||
| # Plan: restore the variant identity on a draft run | ||
|
|
||
| Read research.md first. It proves the mechanism. This document chooses the fix. | ||
|
|
||
| Tracking issue: https://github.com/Agenta-AI/agenta/issues/5162 | ||
|
|
||
| ## The decision in one line | ||
|
|
||
| Stop dropping the variant reference on a dirty run. Keep dropping only the revision | ||
| reference, because the revision is what marks a run as non-draft. The variant says which | ||
| variant is running, and `commit_revision` needs it. | ||
|
|
||
| ## What must stay true | ||
|
|
||
| Any fix has to hold three invariants at once. research.md shows how they interact. | ||
|
|
||
| 1. **A committed-revision run stays non-draft.** A clean run (no unsaved edits) must still | ||
| send all three references, so `is_draft` is false and the run is pinned to its committed | ||
| snapshot. | ||
| 2. **A dirty inline-config run stays a draft.** When the panel has unsaved edits, the run is | ||
| an inline draft. `is_draft` must stay true. Since `is_draft` keys only on the revision | ||
| reference (`tracing.py:165`), the run must not carry a revision reference. | ||
| 3. **`commit_revision` always has a variant target.** The run must carry the variant identity | ||
| whenever a real committed variant exists, draft or not. | ||
|
|
||
| The current code satisfies 1 and 2 but breaks 3, because it drops the variant along with the | ||
| revision. | ||
|
|
||
| ## Option 1 (recommended): fix the frontend read path | ||
|
|
||
| ### What changes | ||
|
|
||
| In `agentRequest.ts`, replace the all-or-nothing gate with a field-level gate. Always forward | ||
| the `application` and `application_variant` references when they exist. Forward | ||
| `application_revision` only when the run is clean (`!isDirty`). | ||
|
|
||
| Today: | ||
|
|
||
| ```typescript | ||
| const references = isCommittedRevisionRun ? fullReferences : null | ||
| ``` | ||
|
|
||
| After (illustrative, final shape decided at implementation): | ||
|
|
||
| ```typescript | ||
| // Always identify WHICH variant is running (well-defined even for a dirty draft). | ||
| // Forward the committed-revision reference ONLY on a clean run, so a dirty run stays | ||
| // is_draft=true. The variant is orthogonal to draft-ness (see research.md). | ||
| const references = fullReferences | ||
| ? { | ||
| ...(fullReferences.application ? {application: fullReferences.application} : {}), | ||
| ...(fullReferences.application_variant | ||
| ? {application_variant: fullReferences.application_variant} | ||
| : {}), | ||
| ...(isCommittedRevisionRun | ||
| ? {application_revision: fullReferences.application_revision} | ||
| : {}), | ||
| } | ||
| : null | ||
| ``` | ||
|
|
||
| ### The reference block, before and after the fix | ||
|
|
||
| For a dirty run of an agent that was committed once: | ||
|
|
||
| - **Before:** `references: null`. `commit_revision` throws. | ||
| - **After:** `references: {application: {...}, application_variant: {...}}`. Run context has | ||
| `variant.id` set and `revision` unset, so `is_draft` is true and the commit works. | ||
|
|
||
| A clean run is unchanged: it still sends all three families and stays non-draft. | ||
|
|
||
| ### Why this is the right layer | ||
|
|
||
| - It matches the repo rule to normalize on the frontend read path. The frontend is where the | ||
| run identity is decided, and it is the layer that knows exactly which variant is loaded. | ||
| - It is small. It touches one expression in one file. | ||
| - It preserves `is_draft` by construction. The draft signal is the revision reference, and | ||
| the fix keeps gating that reference on `!isDirty`. | ||
| - It needs no wire change, no SDK change, no runner change. The backend already reads the | ||
| variant reference it is handed (`tracing.py:155-158`). | ||
|
|
||
| ### Trade-offs and how we cover them | ||
|
|
||
| - **It relies on a verified invariant.** research.md proves that a playground run always | ||
| sends `data.parameters`, so the backend never re-resolves the forwarded variant to a HEAD | ||
| revision (`resolver.py:577-582`). If a future change ever sent a references-only agent run | ||
| with no parameters, forwarding a bare variant would hydrate it to a HEAD revision and flip | ||
| `is_draft` to false. We lock this invariant with a unit test (see the test plan) so a | ||
| regression is caught in CI rather than in production. | ||
| - **A never-committed local draft still cannot self-commit.** `buildAgentReferences` drops | ||
| non-UUID ids, so a brand-new agent that was never saved has no real variant id to forward, | ||
| and `commit_revision` still has no target. This is correct and out of scope: there is no | ||
| variant to commit to yet. The reported loop is about an agent that was committed once and | ||
| then keeps failing, and that agent has a real variant id. We note this edge here so the | ||
| reviewer knows it is deliberate, not missed. | ||
|
|
||
| ## Option 2: derive the variant in the service from the app id | ||
|
|
||
| ### What changes | ||
|
|
||
| The service would look up the variant from `application_id` (which is on the URL query even | ||
| for a draft run, `agentRequest.ts:378-386`) whenever the request carries no explicit variant | ||
| reference. It would then fill `runContext.workflow.variant` from that lookup. | ||
|
|
||
| ### Trade-offs | ||
|
|
||
| - **Larger blast radius.** The change spreads across the SDK reference assembly and the | ||
| service run-context builder, and it adds a database lookup on a path that today reads only | ||
| what the client sent. | ||
| - **Ambiguous for multi-variant apps.** An app can have more than one variant. The app id | ||
| alone does not say which variant is running. The service could guess (for example, the most | ||
| recent variant), but a guess can bind the wrong variant and commit to it. That is a | ||
| correctness risk on a write operation. | ||
| - **Wrong layer for a client-known fact.** The frontend already knows exactly which variant | ||
| is loaded. Recovering that identity in the backend from a weaker signal is more code to do | ||
| a worse job. | ||
|
|
||
| Option 2 does have one attraction: it works even if the frontend never sends a variant. But | ||
| the frontend does know the variant, so that robustness buys little and costs correctness. | ||
|
|
||
| ## Recommendation | ||
|
|
||
| Take **Option 1**. It is the smallest change, it lives in the layer that owns the decision, | ||
|
mmabrouk marked this conversation as resolved.
|
||
| it preserves `is_draft` by construction, and it avoids the multi-variant ambiguity that makes | ||
| Option 2 risky on a write path. Option 2's only edge (surviving a frontend that sends no | ||
| variant) does not apply, because the frontend has the variant and can send it. | ||
|
|
||
| ## What happens after a successful self-commit | ||
|
|
||
| A successful `commit_revision` creates a new revision and moves the HEAD. Two questions | ||
| follow. | ||
|
|
||
| 1. **Does the next commit work without a page reload?** With Option 1, yes. The variant | ||
| identity is forwarded on every run regardless of `isDirty`, so a second commit in the same | ||
| conversation has its target. The loop described in research.md is broken at the source. No | ||
| reload is required for correctness. | ||
| 2. **Should the frontend adopt the new revision so the panel re-syncs and `isDirty` resets?** | ||
| This is a display concern, not a correctness one. After a self-commit, the loaded panel | ||
| still shows the pre-commit inline edits, which now genuinely differ from the new HEAD, so | ||
| the run stays a draft (`is_draft` true). That is honest. Adopting the new revision (reload | ||
| or sync the panel, bump the version chip) would make the UI reflect the new committed state | ||
| and reset `isDirty`. It is a nice follow-up for UI accuracy, but it must stay decoupled | ||
| from the run-context fix. Option 1 makes the run robust whether or not the panel re-syncs, | ||
| so the two concerns do not entangle. We record the panel-adoption follow-up as optional in | ||
| status.md. | ||
|
|
||
| Draft-mode semantics do not regress under Option 1. A clean run sends all three references | ||
| and stays non-draft. A dirty run sends the variant but not the revision and stays a draft. | ||
| The variant identity and the draft flag move independently, which is exactly what | ||
| `tracing.py` already expects. | ||
|
|
||
| ## Adjacent work: server-stamped identity in run context | ||
|
|
||
| A separate design, the session keep-alive project, is landing follow-ups that also enrich the | ||
| run context. Its follow-up 5 (`docs/design/agent-workflows/projects/session-keepalive/status.md`, | ||
| decision 1 and follow-up 5) moves the session pool's project scope off the sandbox mount and | ||
| stamps a server-verified `project_id` into `runContext`. The two designs are adjacent because | ||
| both add identity to `runContext`, but they do not overlap: | ||
|
|
||
| - Follow-up 5 stamps **project identity**, server-side, from request auth state. | ||
| - This design fixes **variant identity**, by forwarding the variant reference that the service | ||
| already resolves into `runContext.workflow`. | ||
|
|
||
| They do not contradict each other. One fills the project scope; the other fills the variant | ||
| scope. | ||
|
|
||
| There is a natural long-term direction that unifies them. Today the variant identity is | ||
| trusted from the client `references` block. The project identity in follow-up 5 is instead | ||
| derived server-side from request state, which is stronger, because the server does not have to | ||
| trust the client for it. The eventual end-state is a single "server-stamped identity in run | ||
| context" where both the project and the workflow or variant identity are derived server-side | ||
| from the authenticated request, so `commit_revision`'s variant binding is immune to whatever | ||
| the client sends. That is a larger change and out of scope here. Option 1 is the correct | ||
| minimal fix now, and it does not block that end-state; it fills the same `runContext.workflow` | ||
| field the server-stamped version would later own. | ||
|
|
||
| ## Acceptance checks | ||
|
|
||
| - A dirty run of a committed agent sends a `references` block that contains `application` and | ||
| `application_variant` and does **not** contain `application_revision`. | ||
| - A clean run of a committed agent still sends all three reference families. | ||
| - The run context for a dirty run has `workflow.variant.id` set and `workflow.is_draft` true. | ||
| - `commit_revision` succeeds on the second and later calls within one conversation, with no | ||
| page reload. | ||
| - A never-committed local draft still sends no variant (unchanged), because there is no real | ||
| variant id yet. | ||
|
|
||
| ## Test plan | ||
|
|
||
| **Unit, frontend (`buildAgentRequest` reference gating).** Add cases to the request-builder | ||
| tests: | ||
|
|
||
| - Clean, committed revision: `references` contains all three families. | ||
| - Dirty, committed revision: `references` contains `application` and `application_variant`, | ||
| and omits `application_revision`. | ||
| - Local draft (non-UUID ids): `references` is null (no variant forwarded), unchanged. | ||
| - Invariant guard: a dirty committed run still sets `data.parameters`, so the backend | ||
| hydration gate never fires. Assert the request carries `data.parameters` alongside the | ||
| bare-variant references, documenting the invariant Option 1 depends on. | ||
|
|
||
| **Integration or replay, the self-commit loop.** Turn the reported failure into a regression | ||
| test using the agent-replay approach (see the `agent-replay-test` skill). Capture a real run | ||
| where the agent commits, then commits again in the same conversation. Assert: | ||
|
|
||
| - The first `commit_revision` succeeds. | ||
| - The second `commit_revision` in the same conversation succeeds (today it fails). | ||
| - The second run's run context still reports `is_draft` true (the draft signal did not | ||
| regress when the variant was forwarded). | ||
|
|
||
| **Live check (optional, during implementation).** Reproduce in the playground on the dev box: | ||
| open a committed agent, ask for two changes in one conversation, confirm both commits land and | ||
| two new versions appear without reloading. | ||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, are we also dropping the application reference, like the workflow reference, and we should not , no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Today the code drops all three families together on a dirty run. The fix keeps application and application_variant and drops only application_revision when the run is dirty, because the revision reference is the one signal that marks a run as non-draft. See the gate change in web/packages/agenta-playground/src/state/execution/agentRequest.ts and plan.md Option 1 (the decision line now names the application reference explicitly).