feat(api): wire ordered operations into the workflow commit endpoint - #5751
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds ordered workflow revision operations with feature gating, base-revision validation, checked commit outcomes, structured warnings, error context, and conditional API side effects. The SDK exposes ordered-operation schemas while legacy commit behavior remains available. ChangesOrdered revision commits
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SDK as commit_revision
participant Router as workflow revision router
participant Service as WorkflowsService
participant ChangeSet as change-set engine
participant Store as revision store
SDK->>Router: Send ordered operations and base_revision_id
Router->>Service: commit_workflow_revision_checked
Service->>ChangeSet: Normalize and apply operations
ChangeSet-->>Service: Return resolved data and warnings
Service->>Store: Create revision when data changed
Service-->>Router: Return CommitOutcome
Router-->>SDK: Return status, revision, and warnings
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
|
|
||
| return _workflow_revisions | ||
|
|
||
| async def commit_workflow_revision_checked( |
There was a problem hiding this comment.
This is a new method. It is not a change to the existing commit method.
Only the workflow commit endpoint calls it. commit_workflow_revision keeps its signature and its behavior for every other caller. Applications, evaluators, and the simple workflow save path continue to use the original method.
If you move this logic into commit_workflow_revision, those callers receive the base check and the no-change answer. None of them expects either one.
| changed=changed, | ||
| ) | ||
|
|
||
| def _check_base_revision( |
There was a problem hiding this comment.
This check reads the head in its own transaction. Two writers can both pass it, so it is not the guarantee.
It is still useful. It refuses the common stale-base case one round trip earlier, and it produces the same 409 body.
The lane above passes this value to the DAO. The DAO then re-reads the head while it holds the variant lock. That read is what refuses the second writer.
| # so it sits beside `delta` and never inside it. Optional: a legacy caller that omits | ||
| # it keeps today's last-write-wins behavior. An ordered delta requires it, and the | ||
| # service enforces that (contract commit-transaction.md section 8). | ||
| base_revision_id: Optional[UUID] = None |
There was a problem hiding this comment.
base_revision_id is a precondition for the commit. It is not part of the change.
For that reason it sits beside delta and never inside it. An ordered delta requires the value, because its text anchors have a meaning only against the revision that the agent read.
A legacy delta keeps the value optional. The shipped playbooks do not send it, and a refusal would stop them.
| warnings = warnings + list(result.warnings) | ||
|
|
||
| # Rejection beats silent stripping: the spike showed that errors teach. | ||
| offenders = find_platform_tool_entries(result.data) |
There was a problem hiding this comment.
The playground adds its own tools to the agent for the duration of a run. Those tools are not part of the stored configuration. Agents commit them by accident today.
The wrapper refuses the commit and names the offending entries. The usability spike showed that an error teaches the model to correct itself. A silent removal of the entries does not.
| # it on adds the ordered arm to the request model and the catalog schema. The flag | ||
| # exists so the API can ship dark, ahead of the SDK catalog and the runner, per the | ||
| # mixed-version rollout order. | ||
| ordered_operations_enabled: bool = _parse_bool_env( |
There was a problem hiding this comment.
This flag keeps the ordered arm out of the model-facing surface. The default value is false.
With the flag off, an ordered delta receives the refusal that any unknown shape receives today. The tool description and the input schema also stay unchanged.
The flag lets the API deploy before the SDK catalog and the runner. Keep it until all three parts are deployed.
85fcb4b to
33255a0
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
api/oss/src/core/workflows/commit_support.py (2)
110-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
key_field_namesfromKEY_FIELDSinstead of hardcoding it.Line 110 hardcodes
{"name", "path", "op", "slug"}. Line 118 tests the enclosing segment against the importedKEY_FIELDS. The two checks read the same registry from two places, and only one of them tracks it.If
change_set.pyadds a fifth keyed list with a new key field,KEY_FIELDSgains the entry and this set does not. Mistake 2 then stops normalizing for the new list, silently. The module docstring inapi/oss/src/core/workflows/service.py(lines 2650-2652) states the same principle for_deep_merge: one home, so the two cannot drift.♻️ Derive the set from the imported registry
- key_field_names = {"name", "path", "op", "slug"} + # Derived, not copied: a new keyed list in `change_set.KEY_FIELDS` must not need an + # edit here to keep this normalization working. + key_field_names = set(KEY_FIELDS.values())Confirm the shape of
KEY_FIELDSbefore applying this. The check on line 118 tests a list name against it, so it appears to map list name to key field.#!/bin/bash # Inspect KEY_FIELDS and item_key in the change-set engine. fd -t f 'change_set.py' api --exec rg -n -B2 -A12 'KEY_FIELDS|def item_key'
298-314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
enrich_reasonhas no caller, no export, and no test.Three observations on this function:
- It is absent from
__all__(lines 27-34), unlike the five helpers beside it.- No caller appears in this PR.
_apply_ordered_deltainapi/oss/src/core/workflows/service.pyraisesChangeSetErrordirectly and never enriches a reason payload, so the "nearest lines" and "available folders" content the module docstring promises for contract 12.4 does not reach any response.reasonis typedstrand is compared toReason.TEXT_NOT_FOUNDandReason.SOURCE_NOT_FOUNDon lines 308 and 312. IfReasonis a plainEnumrather than astrsubclass, both branches are unreachable and enrichment fails silently instead of raising.Confirm whether the wiring is intended in a later slice. If it is, add a TODO naming the slice so the gap is visible. Do you want me to open an issue to track wiring
enrich_reasoninto the ordered-delta error path?#!/bin/bash # Find any caller of enrich_reason and check whether Reason is a str enum. rg -nP --type=py -C3 '\benrich_reason\s*\(' fd -t f 'change_set.py' api --exec rg -n -B2 -A8 'class Reason'api/oss/tests/pytest/unit/workflows/test_commit_support.py (1)
266-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a test for a multi-line
old_text.
nearest_linesreducesold_textto its first non-empty line before scoring (commit_support.pyline 276). No test covers that reduction, so a change to the needle selection would pass this suite while degrading the recovery hint that the function exists to produce.💚 Test the first-line reduction
def test_nearest_lines_survives_empty_input(self): assert nearest_lines("", "x") == [] assert nearest_lines("x", "") == [] + + def test_nearest_lines_anchors_on_the_first_line_of_a_multi_line_anchor(self): + # A stale anchor usually spans several lines. Only its first line locates the + # near miss, so the reduction is the behavior, not an implementation detail. + lines = nearest_lines( + self.TEXT, "Run the release checks manualy.\nThen post the result.\n" + ) + assert lines[0]["line"] == 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ff6c39cc-676f-41df-a171-2be7a63ed669
📒 Files selected for processing (10)
api/oss/src/apis/fastapi/workflows/models.pyapi/oss/src/apis/fastapi/workflows/router.pyapi/oss/src/core/tools/platform_handlers.pyapi/oss/src/core/workflows/commit_support.pyapi/oss/src/core/workflows/dtos.pyapi/oss/src/core/workflows/service.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/unit/tools/test_platform_handlers.pyapi/oss/tests/pytest/unit/workflows/test_commit_support.pysdks/python/agenta/sdk/agents/platform/op_catalog.py
9b80aa9 to
7e94e2a
Compare
33255a0 to
9885e95
Compare
7e94e2a to
39f2a82
Compare
9885e95 to
656f441
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
39f2a82 to
fb6de69
Compare
656f441 to
babd761
Compare
fb6de69 to
9ba89e7
Compare
babd761 to
34d50fc
Compare
9ba89e7 to
5efea5e
Compare
34d50fc to
37b160f
Compare
90a61ea to
97ba90e
Compare
78354e1 to
792e1c1
Compare
97ba90e to
7c79eef
Compare
7c79eef to
0a97166
Compare
792e1c1 to
40dd540
Compare
0a97166 to
616ad42
Compare
The commit endpoint gains the ordered-delta branch through the engine, the optional base_revision_id comparison, no-change detection with status committed|no_change and the complete-revision response, the server-derived commit message with the ephemeral description appended, selector normalization with a target_normalized warning, platform-kind tool rejection, and enriched retryable errors. The legacy set/remove path is untouched and parity-pinned. Strict field checking applies to the new arm only. One flag (ordered_operations_enabled) restores today's surface exactly; the catalog advertises per the flag.
…, contract-true derived message, nearest-lines wired (corrected review blockers)
…p) and the remaining hardening hunks whose regions this lane owns
…20000/50000 levels, match-mode retryable answer, identical unkeyed-list refusals across the three item ops, typed legacy shapes incl. bare-string remove, nearest-lines timing bound
…_operation_shape with a next_step naming the modes; the invalid_match_mode public code is removed (scope-rule simplification, one fewer vocabulary item)
…mitted string (idempotent normalization, platform_guidance_stripped warning, never an error); fence literals pinned cross-side
40dd540 to
34cc7c8
Compare
616ad42 to
06ddcbb
Compare
dosubot
Bot
added
Backend
Feature Request
Railway Preview Environment
|
1 participant
Context
The lane below added the change-set engine. Nothing called it. This lane connects it to the commit endpoint and adds the two answers a read-then-edit loop needs.
Neither answer existed before. A commit built on a revision that is no longer the head was accepted and silently won, so one of two concurrent edits disappeared from the history the user sees. A commit that produced the configuration already stored created a revision anyway, and a commit event throws away the warm sandbox. A model that has been cornered sends exactly that commit to manufacture a success, and the usability spike watched one do it.
What this changes with the flag off
Nothing about the model-facing surface.
AGENTA_WORKFLOWS_ORDERED_OPERATIONS_ENABLEDdefaults to off, and with it off an ordered delta is refused as an unknown shape, which is today's answer. The tool description and schema the model sees are unchanged, the legacysetandremovearm behaves as it always has, and a legacy delta that changes nothing still creates a revision.Three things are true regardless of the flag, because they are correctness on a path that already shipped:
WorkflowRevisionCommitgainsbase_revision_id, the revision the change was built on. It is a precondition, not a mutation, so it sits besidedeltaand never inside it. Sending it is how a caller asks for the staleness check: the commit is refused with 409 when the variant head has moved since. Omitting it keeps today's last-write-wins behavior exactly, so shipped callers are unaffected. An ordered delta requires it.A stale base answers 409 naming the current head, so the caller can re-read that exact revision in one step:
{"detail": { "code": "revision_conflict", "message": "The workflow head changed. No revision was committed.", "base_revision_id": "019c-old", "current_revision_id": "019c-new", "retryable": true }}The no-change comparison and the base check now run inside the same variant lock as the insert, in the DAO from the lane below. Deciding either before the call reads a head another writer can move afterwards: the caller was then told
no_changeabout a revision that was no longer the head, and the 409 the DAO would have raised never happened because the early return skipped it. There is now one decision point, and a moved head beats a no-change answer.What the flag turns on
The ordered arm: seven operations applied in array order, all or nothing, so an agent can change one field without resending its whole configuration. The catalog then offers the ordered form, and the commit
messageleaves the model-facing schema, because the server derives it from the operations and the measurement showed the model corrupts a message it writes itself.The
no_changeanswer is part of that surface and is gated with it. With the flag on, a commit that produces the stored configuration returnsstatus: "no_change"and the current head, publishes no event, and invalidates no cache.Engine hardening
Five defects found by review, all on this lane's own new code, plus a cost bound.
A deeply nested value crashed the server. The engine walks values recursively in several places and
deepcopydoes too, so a deeply nested value raisedRecursionErrorfrom inside the engine, which reaches the caller as a 500: the server reporting that it broke, when what happened is that it will not accept what it was sent. Values nested past 64 levels are now refused withvalue_too_deep. The guard runs before anything else touches the delta, becausedeepcopyand the scope walk are themselves recursive passes and a guard placed after either is a guard the overflow reaches first. It measures depth iteratively, since a recursive depth check is the same overflow one frame earlier.The limit is measured, not guessed. Across 127 stored revisions on the preview database the deepest configuration nests 9 levels, median 9, none above 32. The limit of 64 leaves seven times headroom over the deepest real configuration.
An invented marker was stored as configuration. The embed resolver skips a reference whose value is not an object, so
{"@ag.embed": {"@ag.references": {"file": "/abs/path"}}}resolved to nothing and the literal marker dict was written as the value. A live session sent exactly that, meaning "import this file", was told the commit succeeded, and every read of that field returned a marker afterwards. An@ag.embedmust now be one: a non-empty object holding only@ag.referencesand@ag.selector, with at least one reference, each reference an object. Any other@ag.*key, at any depth, is refused as an unknown marker. Both refusals name the two valid forms, including@ag.filefor pulling in file content, because a model that invents a marker has to be shown the real one.A bad
match_modesaid the verb was wrong.edit_textwith an unknown mode answeredunknown_operation, non-retryably, which tells an agent its operation does not exist and that there is no way forward. The operation was fine and the fix is one word. It is now a retryableinvalid_operation_shapewhosenext_stepnamesautoandexact.replace_itemandremove_itemdid not say a list was unkeyed.add_itemalways answeredunkeyed_collectionprecisely. Its two siblings went to the lookup and reported that the list did not exist, or that no entry matched. Both are true, neither is the reason, and an agent reading them retries the same shape against a list that can never take it. All three now give the same answer.The legacy fields were untyped. A
removesent as a bare string is iterable, so it silently became one removal per character.setmust now be an object andremovea list of strings, checked before either arm touches them.A failed anchor is bounded.
nearest_linesreturns the near misses with a failededit_text, so the agent can re-anchor in the same turn instead of spending one reading the field back. It runs on a refusal, so its cost is paid by a call that produces nothing and an agent can trigger it by mistyping. It now scans at most 5000 lines and clips each line and the anchor to 2000 characters, which also bounds the single-enormous-line case, where comparing against a minified file is quadratic in its length.The same review pass removed a public reason code rather than adding one. The
match_moderefusal reuses the existinginvalid_operation_shapeand carries its guidance onnext_step, which is what that field is for, so the runner and the frontend have one fewer code to learn.Tests
test_change_set.pygains 320 lines: the marker refusals driven by the exact payload from the live session, the depth guard on both delta arms, the unkeyed-list answer across all three item operations, the legacy field shapes, and the bounds on the near-miss search.test_commit_support.py, 250 lines, covering the wrapper-owned jobs: selector normalization, the derived commit message, and the platform-tool rejection.test_ordered_operations_flag.pyasserts that the API and the SDK catalog accept the same spellings, and the catalog half of that agreement only exists on the SDK wiring lane, so the test rides that PR (agent-config-editing-s3b-wire-py). Below it the test has nothing to compare against, fails in isolation, and reddens every PR in between. What it protects is this: the API acceptedenabled,t,yandenableand the catalog did not, so a deployment writtenenabledwould have turned the ordered arm on in the API while the catalog kept advertising the legacy surface, and the model would send a shape it was never shown.commit_workflow_revisionstill behaves identically for its other callers. Applications, evaluators, and the simple-workflow save path all reach it and none routes through the checked wrapper.contracts/change-set.md) landed one lane down in feat(api): add the change-set engine for ordered config edits #5748, so the doc and the code it describes are in different PRs of this stack.This targets
agent-config-editing-s1b-lockand is part of the agent-config-editing stack. Read the stack bottom up.The platform-guidance block
The runner appends a fenced block of platform guidance to the instructions file it renders into the agent's workspace. The stored configuration never contains it: it is injected at render time. A model that copies that rendered file back into a commit would otherwise store our own guidance as the user's configuration, which is how the Luna session ended up with an agent whose instructions described the platform to itself. So the engine removes the block from any committed string, at any depth, in both delta arms. Removal is silent to the model and never an error: it did nothing wrong, and a refusal it has to recover from costs a turn teaching it something it cannot act on. A
platform_guidance_strippedwarning names the field, so the removal is visible to the human reading the response.The strip normalizes rather than matching a fixed newline pattern, which makes it idempotent and means the runner can change its spacing without breaking anything. The round trip is asserted end to end across four different separators: render, copy the rendered file back as a wholesale
set, and the stored text comes back byte for byte. Two delimiter cases are decided rather than left to chance: an unmatched opening fence strips to the end of the string, and a lone closing fence is left as plain text, because it is inert without its opener and deleting on the strength of it would let one stray line remove a user's own content. The two literals are pinned by a test naming the runner'ssystem-prompt-appendix.ts, and the runner's own suite reads this file back, so neither side can change the fence alone. Anedit_textanchor copied out of a stripped region is not special-cased: the block is not in stored text, so the anchor misses and earns the ordinarytext_not_found.