[#5162] fix(frontend): forward variant references on draft playground runs - #5163
Conversation
An agent that edits its own config can commit once, then every later commit in the same conversation fails with 'missing run-context value for direct-call binding workflow_revision.workflow_variant_id'. Root cause: the playground sends references: null on a dirty run, dropping the variant identity that commit_revision needs. Recommends forwarding the variant reference on every run while gating only the committed-revision reference on !isDirty, so is_draft is preserved. Refs #5162 Claude-Session: https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR documents and fixes a bug where repeated ChangesDraft-run references fix and design docs
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
| 1. You load the agent. The panel matches the committed HEAD. `isDirty` is false. | ||
| 2. You ask for a change. The agent calls `commit_revision`. References are sent in full. | ||
| The commit works and creates a **new** revision. The HEAD moves forward. | ||
| 3. The panel you loaded now lags the new HEAD. The dirty check compares the panel against |
There was a problem hiding this comment.
I'm not sure I understand how we are determining this is dirty. What does it mean that we compare to HEAD? What is HEAD actually? I'm not sure here. Is it like a bug? I think it is a new bug where, if there is a commit, then we kind of update the page to the new commit. When there is a regression there and the page has not been updated, maybe that's the source of the issue. That's why it's shown as dirty, but it's not supposed to be.
I want to understand exactly what's the logic there, because obviously dirty means the thing that is loaded has been edited, and probably the thing that is loaded is HEAD, so that makes sense. The issue is apparently it has not been really loaded in the UI, so that's why the comparator is not working. That's my guess.
There was a problem hiding this comment.
Good catch, and the doc was wrong here, so it is rewritten. isDirty does not compare the panel against a newer HEAD. It compares the loaded revision draft overlay against that same revision own immutable server snapshot (web/packages/agenta-entities/src/workflow/state/store.ts:1897-1934), both keyed by the same revision id. So "dirty" means only that the loaded revision carries a draft overlay. Your suspected stale-page-after-commit problem was real historically and is already handled by #4920: the backend emits a data-committed-revision event and the chat panel calls switchEntity to repoint onto the new revision id, which has no overlay, so isDirty resets. The residual risk is that the event can be missed or the stream aborted, which is one more reason the run must carry the variant on every call. Full detail in research.md, "What dirty means, and why the loop happens".
|
|
||
| ## The decision in one line | ||
|
|
||
| Stop dropping the variant reference on a dirty run. Keep dropping only the revision |
There was a problem hiding this comment.
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.
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).
…rd implementation
mmabrouk
changed the title
|
@coderabbitai review |
dosubot
Bot
added
the
size:XL
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/packages/agenta-playground/src/state/execution/agentRequest.ts (1)
342-354: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCondense the reference-gate comment to comply with the one-line comment guideline.
The constraint being documented is genuinely surprising (cross-layer draft-ness derivation, bare-variant safety), but 13 lines exceeds what qualifies as a "brief exception." As per coding guidelines, "Keep in-code comments to one short line maximum unless a genuinely surprising constraint requires a brief exception."
♻️ Suggested condensed comment
- // Field-level reference gate. The service derives draft-ness purely from the revision - // reference — `services/oss/src/agent/tracing.py` `_run_context_workflow`: - // `is_draft = revision is None` — so ONLY `application_revision` is gated on cleanliness: - // - dirty (unsaved left-panel edits) or an uncommitted local draft (no real revision UUID): - // the run is an inline-config draft, so the revision reference is withheld to keep - // `is_draft` true. - // `application` and `application_variant` are forwarded whenever they exist, independent of - // dirtiness: the variant identifies WHICH variant is running, which is orthogonal to - // draft-ness, and a self-targeting tool (e.g. `commit_revision`) needs that variant to bind - // to even on a draft run. Forwarding a bare variant does not resurrect `is_draft`: the - // backend only re-resolves a variant reference to its HEAD revision when the request carries - // no `data.parameters` (`resolver.py` `needs_reference_hydration`), and a playground run - // always sends `data.parameters` — see the "invariant guard" test for this file. + // Field-level reference gate: only `application_revision` is gated on cleanliness + // (backend: `is_draft = revision is None`). `application`/`application_variant` always + // forward — the variant is orthogonal to draft-ness and needed by `commit_revision`. + // A bare variant won't resurrect `is_draft`: the backend only re-resolves it when + // `data.parameters` is absent, which a playground run never is.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 02e3d82e-8577-49f1-8d8b-3b8c24ab4aed
📒 Files selected for processing (7)
docs/design/agent-workflows/projects/draft-run-references/README.mddocs/design/agent-workflows/projects/draft-run-references/context.mddocs/design/agent-workflows/projects/draft-run-references/plan.mddocs/design/agent-workflows/projects/draft-run-references/research.mddocs/design/agent-workflows/projects/draft-run-references/status.mdweb/packages/agenta-playground/src/state/execution/agentRequest.tsweb/packages/agenta-playground/tests/unit/agentRequest.test.ts
1 participant
Context
When an agent commits itself in the playground, the first
commit_revisionworks, but everylater
commit_revisionin the same conversation fails with:The cause is on the frontend.
commit_revisionis a platform op whose target variant is acontext binding, filled from the run's
referencesblock, never by the model. The requestbuilder gated that whole block on an all-or-nothing rule: if the loaded revision had unsaved
edits (
isDirty), it sentreferences: null. With no references, the run context has novariant, and the runner throws.
"Dirty" here does not mean the panel lags a newer revision. It means the loaded revision
carries a draft overlay, for any reason: the user edited the config panel, or the panel had
edits before the conversation started, or the post-commit repointing added in #4920 did not
land. Whenever that was true, the old gate dropped the variant and the commit failed.
Changes
The gate is now field-level instead of all-or-nothing.
applicationandapplication_variantforward whenever they exist.
application_revisionforwards only on a clean run, because therevision reference is the one signal that marks a run as non-draft (
is_draft = revision is Nonein the SDK). An empty result still falls back tonull.The
referencesblock on a dirty run of a committed agent:Before:
After:
The run context now has
variant.idset andrevisionunset, sois_draftstays true andcommit_revisionhas its target. A clean run is unchanged: it still sends all three familiesand stays non-draft.
This lives on the frontend read path because the frontend owns the run identity and knows
exactly which variant is loaded. It is one gate expression in one file, plus tests. No wire,
SDK, or runner change: the backend already reads the variant reference it is handed.
The design docs on this PR carry the full mechanism, the reasoning for the frontend layer, and
the verification that forwarding a bare variant does not re-resolve it to a HEAD revision:
docs/design/agent-workflows/projects/draft-run-references/.Scope / risk
variant, because it has no real variant id yet. That case is unchanged and out of scope.
the event is missed, the panel stays on the old revision and reads as dirty. This fix makes
the run correct in that case; tightening the repointing is a separate display concern.
second commit in one conversation), noted in status.md.
Tests
web/packages/agenta-playground/tests/unit/agentRequest.test.ts: dirty committed run(references carry app + variant, omit revision), truly-uncommitted local draft (references
null), a mixed case (local draft revision id under a committed variant still forwards app +
variant), and an invariant guard (a dirty run still carries
data.parameters, so thebackend hydration gate never fires).
pnpm lint-fixclean.How to QA
Prerequisites: local dev stack, or any environment with an agent app that has been
committed at least once.
Steps:
Expected result: Both commits land and two new versions appear, with no page reload. The
second commit no longer fails with the
workflow_revision.workflow_variant_idbinding error.Automated tests:
Edge cases: A clean run (no unsaved edits) must still send all three reference families
and stay non-draft. A brand-new agent that was never committed still cannot self-commit, which
is correct: there is no variant to target yet.
Closes #5162.
https://claude.ai/code/session_01CSTSEXSe4DDhoXCFjZpZ5W