feat(api): add the change-set engine for ordered config edits - #5748
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a dependency-free change-set engine for legacy and ordered configuration deltas. It supports scoped updates, text edits, keyed-list operations, duplicate detection, warnings, validation, atomicity, and structured retryable errors. ChangesChange-set engine
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant apply_change_set
participant OperationEngine
participant Validator
Caller->>apply_change_set: submit base and delta
apply_change_set->>OperationEngine: validate scope and execute operations
OperationEngine-->>apply_change_set: return copied configuration and warnings
apply_change_set->>Validator: validate final configuration when configured
Validator-->>apply_change_set: return validation result
apply_change_set-->>Caller: return ChangeSetResult or ChangeSetError
🚥 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 |
| policy._prefix = tuple(wanted) # type: ignore[attr-defined] | ||
| # The legacy arm walks the `set` tree only this deep before it asks the policy, so a | ||
| # refused sub-path deeper than the prefix would never be reached and never refused. | ||
| policy._prefix_depth = max( # type: ignore[attr-defined] |
There was a problem hiding this comment.
The legacy arm does not have one target for each operation. It walks the set tree and asks the policy at this depth.
The refused sub-paths are deeper than the allowed prefix. The prefix is parameters.agent, which is two segments. parameters.agent.sandbox.permissions is four.
If you set this value back to len(wanted), the walk stops at parameters.agent. That target is allowed. A legacy write of the sandbox permissions then passes the scope check.
| self.branch_paths: List[Tuple[PathElement, ...]] = [] | ||
|
|
||
| @staticmethod | ||
| def _plain(segments: Sequence[Segment]) -> Tuple[PathElement, ...]: |
There was a problem hiding this comment.
A selector names one entry of a list. This function keeps the entry key together with the list name. The result is a path element such as ("skills", "alpha").
Do not flatten a selector to its list name. If you flatten it, the path of skills[alpha].files becomes equal to the path of skills[beta].files. The unique-name check then applies the state of one skill to every other skill.
| return item_key(list_name, entry) or f"#{index}" | ||
|
|
||
|
|
||
| def _collections( |
There was a problem hiding this comment.
This function gives each nested collection its own identity. An entry of a keyed list adds its key to the path. An entry of an unkeyed list keeps the path of its parent, because no selector can address it.
The dictionary key must stay unique for each collection. found.update keeps only the last value for a repeated key. If two collections share a key, the duplicates in every entry except the last one become invisible.
| def item(self, segments: Sequence[Segment]) -> None: | ||
| self.item_paths.append(self._plain(segments)) | ||
|
|
||
| def branch(self, segments: Sequence[Segment]) -> None: |
There was a problem hiding this comment.
A write inside a selected entry also touches the list that holds the entry. The write can change the key field of that entry. The new key can be equal to the key of a sibling.
This loop records the parent list as touched for that reason. If you remove the loop, a rename that creates a duplicate is accepted with a warning only.
| if not after: | ||
| continue | ||
| was = before.get(path, {}) | ||
| item_touched = any(p == path for p in touched.item_paths) |
There was a problem hiding this comment.
The check has three tiers. An item operation must leave its collection clean. A branch write must not increase a duplicate count. An untouched collection gives a warning only.
The third tier is necessary. Configurations with duplicate names exist in production. Without that tier, a user cannot edit those configurations again.
|
@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: 3
🧹 Nitpick comments (10)
api/oss/src/core/workflows/change_set.py (5)
11-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the count in the docstring, and consider renaming
Warning.Line 11 says "Three things", but the sentence lists four items: selector normalization, platform-tool rejection, the derived commit message, and the enriched error content.
Separately, the public class
Warningshadows the builtinWarning. The module exports it through__all__, so a star-import in a consumer replaces the builtin in that namespace. ConsiderChangeSetWarningfor the public name.
341-353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
deep_mergereturns a shallow-aliasing result.
deep_mergecopies only the top level. When a key is absent frombase, the merged dict binds the value frompatchby reference. Nested dicts, lists, and strings are then shared withpatch.This is correct for parity with
service.py, but the aliasing matters for the legacy arm inapply_change_set. See the separate comment on lines 1386-1394. Add a note to the docstring so a future caller does not assume a deep copy.
459-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMake the traversal depth part of the
ScopePolicycontract.
subtree_scopeattaches_prefixand_prefix_depthto the returned function._policy_depthand_scope_next_stepread those attributes withgetattrand silently fall back to depth1and an empty prefix.
ScopePolicyis typed as a plainCallable, so nothing tells a caller that a custom policy must carry these attributes. A hand-written policy that refuses paths deeper than one segment is then asked only about top-level keys in the legacy arm, and a deep legacysetpasses the check.Both shipped policies come from
subtree_scope, so this is not a current defect. Consider a small dataclass or aProtocolwith explicitprefixandprefix_depthmembers so the requirement is visible in the type.
934-959: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReject unknown keys on an operation, as
_classifydoes for the delta.
_classifyrefuses unknown delta fields at line 1284. No equivalent check exists for an operation object._apply_operationreadsoperation,target,value, and the per-verbeditsandmatch_mode, and ignores every other key.A misspelled key is then silent. An agent that sends
match_modes: "exact"receivesautotolerance, so a normalized match can rewrite a span the agent asked to match byte-exactly.INVALID_OPERATION_SHAPEalready exists for this case and is retryable.♻️ Proposed check
+_OPERATION_FIELDS = {"operation", "target", "value", "edits", "match_mode"} + def _apply_operation( root: Dict[str, Any], operation: Dict[str, Any], warnings: List[Warning], index: int, touched: "_Touched", ) -> None: verb = operation.get("operation") if verb not in OPERATIONS: raise _Fail( Reason.UNKNOWN_OPERATION, f"unknown operation {verb!r} (known: {', '.join(OPERATIONS)})", ) + unknown = set(operation) - _OPERATION_FIELDS + if unknown: + raise _Fail( + Reason.INVALID_OPERATION_SHAPE, + f"unknown operation fields: {', '.join(sorted(unknown))}", + )
1108-1108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider including
filesin the wholesale-replace warning.
KEY_FIELDSnames four keyed lists:skills,mcps,files, andtools._warn_wholesalechecks only three. Asetthat replaces a skill's wholefileslist therefore emits noWHOLESALE_LIST_REPLACEwarning, althoughadd_item,replace_item, andremove_itemall addressfilesentries.If the contract deliberately names only three lists, add a short comment here so the omission reads as intentional.
api/oss/tests/pytest/unit/workflows/test_change_set.py (5)
169-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused second element of
LEGACY_CASES.Every entry in
LEGACY_CASESis a two-tuple whose second element isNone, andtest_same_result_as_the_original_foldbinds it to_unused. Parametrize over the delta alone.- `@pytest.mark.parametrize`("delta,_unused", LEGACY_CASES) - def test_same_result_as_the_original_fold(self, delta, _unused): + `@pytest.mark.parametrize`("delta", LEGACY_CASES) + def test_same_result_as_the_original_fold(self, delta): assert apply(delta) == reference_legacy_apply(base_config(), delta)Also at line 206:
into.get(key) is not Noneis redundant, becauseisinstance(None, dict)is alreadyFalse.
625-626: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the concrete exception type instead of
Exception.These tests use
pytest.raises(Exception)and then readcaught.value.reason. The broad type does not state which exception the contract promises, and it accepts any unrelated failure that happens to carry areasonattribute.
apply_text_editsraises the private_Fail. The file already imports private names where it needs them, at line 1724 for_foldand line 2100 forNEXT_STEPS. Use the same approach here, or add_Failto the module's__all__if the direct-call contract is meant to be public.♻️ Proposed change
+from oss.src.core.workflows.change_set import _Fail + ... - with pytest.raises(Exception) as caught: + with pytest.raises(_Fail) as caught: apply_text_edits("abc", [{"old_text": "", "new_text": "x"}]) assert caught.value.reason == Reason.EMPTY_OLD_TEXTAlso applies to: 636-637, 641-642, 646-647, 651-652, 662-663, 685-686
735-738: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse explicit escapes for the invisible characters.
test_a_non_breaking_space_does_not_match_a_spaceshows"a b"for both the text and the anchor.test_composed_and_decomposed_unicode_do_not_matchshows"café"for both. The distinction is a U+00A0 in the first case and a combining acute in the second, and neither is visible in the source.Write the non-ASCII code point as an escape, so a reader sees what the test asserts and an editor that normalizes on save cannot alter it.
♻️ Proposed change
def test_a_non_breaking_space_does_not_match_a_space(self): with pytest.raises(Exception) as caught: - apply_text_edits("a b", [{"old_text": "a b", "new_text": "x"}]) + apply_text_edits("a\u00a0b", [{"old_text": "a b", "new_text": "x"}]) assert caught.value.reason == Reason.TEXT_NOT_FOUND def test_composed_and_decomposed_unicode_do_not_match(self): - # "é" as one code point vs. "e" + combining acute. with pytest.raises(Exception) as caught: - apply_text_edits("café", [{"old_text": "café", "new_text": "x"}]) + # "é" as one code point vs. "e" + combining acute. + apply_text_edits("caf\u00e9", [{"old_text": "cafe\u0301", "new_text": "x"}]) assert caught.value.reason == Reason.TEXT_NOT_FOUNDApply the same treatment to the U+FEFF in
test_a_bom_is_not_strippedat line 751.Also applies to: 753-757
2110-2136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the limits from the engine constants.
Line 2112 hard-codes
200_001and line 2133 hard-codesrange(65). Both duplicateMAX_TEXT_LENGTHandMAX_OPERATIONS. If either constant changes, these tests keep passing for the wrong reason, or fail without naming the cause.♻️ Proposed change
-from oss.src.core.workflows.change_set import ( +from oss.src.core.workflows.change_set import ( AGENT_COMMIT_SCOPE, ChangeSetError, + MAX_OPERATIONS, + MAX_TEXT_LENGTH, PARAMETERS_ONLY,- base["parameters"]["agent"]["instructions"]["agents_md"] = "a" * 200_001 + base["parameters"]["agent"]["instructions"]["agents_md"] = ( + "a" * (MAX_TEXT_LENGTH + 1) + )- for i in range(65) + for i in range(MAX_OPERATIONS + 1)
MAX_TEXT_LENGTHandMAX_OPERATIONSare not in the module's__all__, so add them there as well if the star-import surface matters.
2110-2136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the three untested limits.
Contract 5.6.3 declares five limits. The suite covers
MAX_TEXT_LENGTHat line 2110 andMAX_OPERATIONSat line 2125. Three have no test:
MAX_TARGET_SEGMENTS(12): a target of 13 segments must raiseINVALID_TARGET_SHAPE.MAX_EDITS_PER_OPERATION(32): 33 edits in oneedit_textmust raiseINVALID_OPERATION_SHAPE.MAX_OLD_TEXT_LENGTH(20,000): anold_textof 20,001 characters must raiseINVALID_OPERATION_SHAPE.Each is one short test in the same style as the two that exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56fdb3e6-dd03-4a34-8910-64ab38605b59
📒 Files selected for processing (2)
api/oss/src/core/workflows/change_set.pyapi/oss/tests/pytest/unit/workflows/test_change_set.py
216ab63 to
447c544
Compare
212e428 to
690283b
Compare
447c544 to
829b266
Compare
690283b to
df8b834
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. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
api/oss/tests/pytest/unit/workflows/test_change_set.py (4)
625-687: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow
pytest.raises(Exception)toChangeSetError.Each of these blocks reads
caught.value.reasonafterwards, so the expected type isChangeSetError.Exceptionalso accepts an unrelated failure that happens to expose areasonattribute. The module already importsChangeSetError, and thefailurehelper uses it.The same pattern repeats at lines 636, 641, 646, 651, 662, 685, 726, 731, 736, 741, 746, 755, and 763.
♻️ Example for one site
- with pytest.raises(Exception) as caught: + with pytest.raises(ChangeSetError) as caught: edit_text( "one two",
735-757: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrite the invisible code points as escapes.
These three tests depend on characters that a reader cannot see and an editor can silently rewrite: the non-breaking space at line 737, the BOM at line 751, and the decomposed "é" at line 756. If a formatter or a copy-paste normalizes them, the BOM test and the Unicode-normalization test pass without testing anything.
Use explicit escapes so the intent stays visible and stable.
♻️ Proposed change
def test_a_non_breaking_space_does_not_match_a_space(self): with pytest.raises(ChangeSetError) as caught: - apply_text_edits("a b", [{"old_text": "a b", "new_text": "x"}]) + apply_text_edits("a\u00a0b", [{"old_text": "a b", "new_text": "x"}]) assert caught.value.reason == Reason.TEXT_NOT_FOUNDdef test_a_bom_is_not_stripped(self): - assert edit_text("abc", [{"old_text": "abc", "new_text": "x"}]) == "x" + assert edit_text("\ufeffabc", [{"old_text": "\ufeffabc", "new_text": "x"}]) == "x"def test_composed_and_decomposed_unicode_do_not_match(self): # "é" as one code point vs. "e" + combining acute. with pytest.raises(ChangeSetError) as caught: - apply_text_edits("café", [{"old_text": "café", "new_text": "x"}]) + apply_text_edits("caf\u00e9", [{"old_text": "cafe\u0301", "new_text": "x"}]) assert caught.value.reason == Reason.TEXT_NOT_FOUND
1677-1690: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the edited text, not the truthiness of the result.
assert apply(...)passes for any non-empty result dictionary. The test does not confirm that the default mode applied the normalized match. Assert the resultingagents_mdvalue, as the neighbouring test at lines 1541-1557 does.♻️ Proposed change
# No match_mode field at all behaves like "auto". - assert apply( + result = apply( ops( { "operation": "edit_text", "target": AGENT + ["instructions", "agents_md"], "edits": [{"old_text": ASCII_ANCHOR, "new_text": "Run the gate."}], } ), base, ) + assert ( + result["parameters"]["agent"]["instructions"]["agents_md"] + == "Run the gate.\n" + )
2125-2136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the change set limits instead of duplicating them.
The tests hardcode
200_001,200_000, and the operation count boundary whilechange_set.pyownsMAX_TEXT_LENGTH,MAX_OLD_TEXT_LENGTH, andMAX_OPERATIONS. Use the imported constants in these assertions so limit changes remain intentional and do not silently make the tests pass at the old contract.api/oss/src/core/workflows/change_set.py (1)
1121-1139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the wholesale-warning list names from
KEY_FIELDS.
_warn_wholesalematches("tools", "skills", "mcps").KEY_FIELDSholds a fourth keyed list,files. Asetthat replaces a skill's wholefileslist therefore produces noWHOLESALE_LIST_REPLACEwarning, althoughadd_item,replace_item, andremove_itemall addressfilesby name.The same literal is repeated at Line 1503 in the legacy arm, so the two sites can drift.
♻️ Proposed refactor
+# Every name-addressed list. A wholesale replacement of any of them loses the per-entry +# identity that add_item / replace_item / remove_item rely on. +_KEYED_LIST_NAMES = tuple(sorted(KEY_FIELDS)) + + def _warn_wholesale( name: Any, value: Any, warnings: List[Warning], index: int, segments: Sequence[Segment], ) -> None: - if name in ("tools", "skills", "mcps") and isinstance(value, list): + if name in _KEYED_LIST_NAMES and isinstance(value, list):Then use
_KEYED_LIST_NAMESat Line 1503 in place of the repeated literal.Confirm the contract intends
filesto carry this warning before you apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ce5b5b42-0b36-4506-bf4f-e26f5fb75f99
📒 Files selected for processing (2)
api/oss/src/core/workflows/change_set.pyapi/oss/tests/pytest/unit/workflows/test_change_set.py
| def find_file_markers(value: Any, pointer: str = "") -> List[str]: | ||
| """Every ``@ag.file`` marker inside a value, as JSON Pointers. | ||
|
|
||
| The runner keys one execution-authorization record per marker, so the pointer is the | ||
| identity the engine reports when it finds one that survived. | ||
| """ | ||
| found: List[str] = [] | ||
| if isinstance(value, dict): | ||
| if FILE_MARKER in value: | ||
| found.append(pointer or "/") | ||
| for key, child in value.items(): | ||
| found.extend(find_file_markers(child, f"{pointer}/{_escape(key)}")) | ||
| elif isinstance(value, list): | ||
| for index, child in enumerate(value): | ||
| found.extend(find_file_markers(child, f"{pointer}/{index}")) | ||
| return found |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for existing depth or nesting limits in the API request layer.
rg -nP --type=py -C3 '\b(max_depth|MAX_DEPTH|nesting|recursion|setrecursionlimit)\b' api
# Locate the wrappers that will call the engine.
rg -nP --type=py -C3 '\bapply_change_set\b' apiRepository: Agenta-AI/agenta
Length of output: 48776
🏁 Script executed:
#!/bin/bash
set -u
echo "== change_set relevant lines =="
sed -n '310,560p' api/oss/src/core/workflows/change_set.py
echo
sed -n '1400,1510p' api/oss/src/core/workflows/change_set.py
echo
echo "== exact apply_change_set call sites in Python =="
rg -n --type=py 'apply_change_set\(' .
echo
echo "== endpoints/models names mentioning change_set / change-set / diff in workflows =="
rg -n --type=py -C2 'change[_-]?set|delta|diff|ChangeSet|changeSet' api/oss/src api/oss/tests -g '*.py' | head -n 240Repository: Agenta-AI/agenta
Length of output: 34790
Return ChangeSetError when the value traversal recurses too deep.
find_file_markers, deep_merge, and several other helpers recurse into caller-supplied dicts/lists without a depth cap. A deeply nested value can therefore raise RecursionError, which ChangeSetError callers expect not to reach. Add a bounded traversal depth and return a refusal from ChangeSetError.
There was a problem hiding this comment.
Acknowledged, fix queued. Confirmed: deep_merge, find_file_markers, and _legacy_scope_targets all recurse into caller-supplied dicts/lists with no depth cap anywhere in the file. A deeply nested value can raise RecursionError instead of a clean ChangeSetError refusal.
There was a problem hiding this comment.
@mmabrouk, thank you for confirming the affected recursive helpers.
A depth limit should cover all three paths and convert excess nesting into a ChangeSetError refusal. The review remains open until the fix is implemented and verified.
You are interacting with an AI system.
There was a problem hiding this comment.
Acknowledged, fix queued. Confirmed: deep_merge, remove_path, and find_file_markers have no depth guard anywhere in the stack (checked through the tip, agent-config-editing-s3b-wire-web) and there is no max_depth/RecursionError handling in change_set.py. A deeply nested delta value can still raise RecursionError instead of a structured ChangeSetError.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 37 minutes and 55 seconds before sending another message.
| if verb == "edit_text": | ||
| _require_field_tail(segments, "edit_text") | ||
| mode = operation.get("match_mode", "auto") | ||
| if mode not in MATCH_MODES: | ||
| raise _Fail( | ||
| Reason.UNKNOWN_OPERATION, | ||
| f"unknown match_mode {mode!r} (known: {', '.join(MATCH_MODES)})", | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report an invalid match_mode with a retryable reason code.
Reason.UNKNOWN_OPERATION is in _NOT_RETRYABLE (Line 84) and has no NEXT_STEPS entry. An agent that sends match_mode: "fuzzy" therefore receives retryable: false and no next step, so it stops instead of correcting one token. The operation verb is also valid here, so the code names the wrong field.
Reason.INVALID_OPERATION_SHAPE is retryable and its next step already reads "Correct the operation to the shape in the tool description and send it again."
🐛 Proposed fix
mode = operation.get("match_mode", "auto")
if mode not in MATCH_MODES:
raise _Fail(
- Reason.UNKNOWN_OPERATION,
+ Reason.INVALID_OPERATION_SHAPE,
f"unknown match_mode {mode!r} (known: {', '.join(MATCH_MODES)})",
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if verb == "edit_text": | |
| _require_field_tail(segments, "edit_text") | |
| mode = operation.get("match_mode", "auto") | |
| if mode not in MATCH_MODES: | |
| raise _Fail( | |
| Reason.UNKNOWN_OPERATION, | |
| f"unknown match_mode {mode!r} (known: {', '.join(MATCH_MODES)})", | |
| ) | |
| if verb == "edit_text": | |
| _require_field_tail(segments, "edit_text") | |
| mode = operation.get("match_mode", "auto") | |
| if mode not in MATCH_MODES: | |
| raise _Fail( | |
| Reason.INVALID_OPERATION_SHAPE, | |
| f"unknown match_mode {mode!r} (known: {', '.join(MATCH_MODES)})", | |
| ) |
There was a problem hiding this comment.
Acknowledged, fix queued. Confirmed still present at line 1028: an invalid match_mode raises Reason.UNKNOWN_OPERATION, which is in _NOT_RETRYABLE and has no NEXT_STEPS entry, so the agent gets retryable: false with no guidance for a one-token typo. INVALID_OPERATION_SHAPE is retryable and already has a next-step message.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 40 minutes and 36 seconds before sending another message.
There was a problem hiding this comment.
Acknowledged, fix queued. Confirmed still current at line 1028 (and at the tip of the stack): an unknown match_mode raises Reason.UNKNOWN_OPERATION, which is in _NOT_RETRYABLE and has no NEXT_STEPS entry, so the agent gets retryable: false and no next step for a one-token fix. INVALID_OPERATION_SHAPE is retryable and already has the right next-step text.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 37 minutes and 54 seconds before sending another message.
| if verb == "replace_item": | ||
| _require_selector_tail(segments, "replace_item") | ||
| selector = segments[-1] | ||
| list_name, key = selector["list"], selector["key"] | ||
| parent = _parent_of(root, segments, create=False) | ||
| collection, position = _find_item( | ||
| parent, list_name, key, where=f"target segment {len(segments) - 1}" | ||
| ) | ||
| new_key = _derived_key(list_name, value, "replace_item") | ||
| if new_key != key: | ||
| raise _Fail( | ||
| Reason.ITEM_RENAME_NOT_ALLOWED, | ||
| f"the target names {key!r} but the value is named {new_key!r}.", | ||
| ) | ||
| collection[position] = deepcopy(value) | ||
| touched.item(segments[:-1] + [list_name]) | ||
| return | ||
|
|
||
| _require_selector_tail(segments, "remove_item") | ||
| selector = segments[-1] | ||
| parent = _parent_of(root, segments, create=False) | ||
| collection, position = _find_item( | ||
| parent, | ||
| selector["list"], | ||
| selector["key"], | ||
| where=f"target segment {len(segments) - 1}", | ||
| ) | ||
| del collection[position] | ||
| touched.item(segments[:-1] + [selector["list"]]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Refuse replace_item and remove_item on an unkeyed list with UNKEYED_COLLECTION.
add_item checks list_name not in KEY_FIELDS at Line 1068 and raises Reason.UNKEYED_COLLECTION. replace_item and remove_item apply no such check. They call _find_item, which compares item_key(list_name, entry) against the selector key. For a list outside KEY_FIELDS, item_key returns None for every entry, so no comparison matches and _find_item raises Reason.ITEM_NOT_FOUND.
The next step for ITEM_NOT_FOUND tells the agent to call read_config and retry with a key from that list. No key exists for an unkeyed list, so the agent retries a request that can never succeed. UNKEYED_COLLECTION instead tells it to use set.
🐛 Proposed fix
+def _require_keyed_list(list_name: str) -> None:
+ if list_name not in KEY_FIELDS:
+ raise _Fail(
+ Reason.UNKEYED_COLLECTION,
+ f"'{list_name}' is not a name-addressed list "
+ f"(known: {', '.join(sorted(KEY_FIELDS))})",
+ )
+
+
def _apply_operation( if verb == "replace_item":
_require_selector_tail(segments, "replace_item")
selector = segments[-1]
list_name, key = selector["list"], selector["key"]
+ _require_keyed_list(list_name)
parent = _parent_of(root, segments, create=False) _require_selector_tail(segments, "remove_item")
selector = segments[-1]
+ _require_keyed_list(selector["list"])
parent = _parent_of(root, segments, create=False)add_item can then call the same helper in place of its inline check at Lines 1068-1073.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if verb == "replace_item": | |
| _require_selector_tail(segments, "replace_item") | |
| selector = segments[-1] | |
| list_name, key = selector["list"], selector["key"] | |
| parent = _parent_of(root, segments, create=False) | |
| collection, position = _find_item( | |
| parent, list_name, key, where=f"target segment {len(segments) - 1}" | |
| ) | |
| new_key = _derived_key(list_name, value, "replace_item") | |
| if new_key != key: | |
| raise _Fail( | |
| Reason.ITEM_RENAME_NOT_ALLOWED, | |
| f"the target names {key!r} but the value is named {new_key!r}.", | |
| ) | |
| collection[position] = deepcopy(value) | |
| touched.item(segments[:-1] + [list_name]) | |
| return | |
| _require_selector_tail(segments, "remove_item") | |
| selector = segments[-1] | |
| parent = _parent_of(root, segments, create=False) | |
| collection, position = _find_item( | |
| parent, | |
| selector["list"], | |
| selector["key"], | |
| where=f"target segment {len(segments) - 1}", | |
| ) | |
| del collection[position] | |
| touched.item(segments[:-1] + [selector["list"]]) | |
| if verb == "replace_item": | |
| _require_selector_tail(segments, "replace_item") | |
| selector = segments[-1] | |
| list_name, key = selector["list"], selector["key"] | |
| _require_keyed_list(list_name) | |
| parent = _parent_of(root, segments, create=False) | |
| collection, position = _find_item( | |
| parent, list_name, key, where=f"target segment {len(segments) - 1}" | |
| ) | |
| new_key = _derived_key(list_name, value, "replace_item") | |
| if new_key != key: | |
| raise _Fail( | |
| Reason.ITEM_RENAME_NOT_ALLOWED, | |
| f"the target names {key!r} but the value is named {new_key!r}.", | |
| ) | |
| collection[position] = deepcopy(value) | |
| touched.item(segments[:-1] + [list_name]) | |
| return | |
| _require_selector_tail(segments, "remove_item") | |
| selector = segments[-1] | |
| _require_keyed_list(selector["list"]) | |
| parent = _parent_of(root, segments, create=False) | |
| collection, position = _find_item( | |
| parent, | |
| selector["list"], | |
| selector["key"], | |
| where=f"target segment {len(segments) - 1}", | |
| ) | |
| del collection[position] | |
| touched.item(segments[:-1] + [selector["list"]]) |
There was a problem hiding this comment.
Acknowledged, fix queued. Confirmed: add_item checks list_name not in KEY_FIELDS and raises UNKEYED_COLLECTION (line 1068), but replace_item and remove_item still call _find_item directly with no such check, so they fall through to ITEM_NOT_FOUND on an unkeyed list, which tells the agent to retry with a key that can never exist.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 40 minutes and 31 seconds before sending another message.
There was a problem hiding this comment.
Acknowledged, fix queued. Confirmed: add_item checks list_name not in KEY_FIELDS at line 1068 and raises UNKEYED_COLLECTION, but replace_item and remove_item call _find_item directly with no such check, still true at the tip of the stack. For an unkeyed list, item_key returns None for every entry, so _find_item always raises ITEM_NOT_FOUND, whose next step (call read_config, retry with a key) can never succeed for that list.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 37 minutes and 53 seconds before sending another message.
| def _classify(delta: Dict[str, Any]) -> str: | ||
| if not isinstance(delta, dict): | ||
| raise ChangeSetError(Reason.INVALID_DELTA, "the delta must be an object") | ||
| unknown = set(delta) - {"set", "remove", "operations"} | ||
| if unknown: | ||
| raise ChangeSetError( | ||
| Reason.INVALID_DELTA, | ||
| f"unknown delta fields: {', '.join(sorted(unknown))}", | ||
| ) | ||
| has_legacy = any(delta.get(name) is not None for name in _LEGACY_FIELDS) | ||
| has_ordered = delta.get("operations") is not None | ||
| if has_legacy and has_ordered: | ||
| raise ChangeSetError( | ||
| Reason.INVALID_DELTA, | ||
| "a delta uses either 'set'/'remove' or 'operations', never both.", | ||
| ) | ||
| if has_ordered: | ||
| return "ordered" | ||
| if has_legacy: | ||
| return "legacy" | ||
| raise ChangeSetError( | ||
| Reason.INVALID_DELTA, | ||
| "the delta is empty: give 'set'/'remove', or 'operations'.", | ||
| ) | ||
|
|
||
|
|
||
| def _legacy_scope_targets(delta: Dict[str, Any], depth: int) -> List[Target]: | ||
| targets: List[Target] = [] | ||
|
|
||
| def walk(node: Any, path: List[str]) -> None: | ||
| if len(path) >= depth or not isinstance(node, dict) or not node: | ||
| targets.append(list(path)) | ||
| return | ||
| for key, value in node.items(): | ||
| walk(value, path + [key]) | ||
|
|
||
| for key, value in (delta.get("set") or {}).items(): | ||
| walk(value, [key]) | ||
| for path in delta.get("remove") or []: | ||
| targets.append(path.split(".")) | ||
| return targets |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for a request model that constrains the legacy delta fields.
rg -nP --type=py -C4 '\b(set|remove|operations)\b.*=.*Field\(' api -g '**/dtos*.py' -g '**/models*.py'
# Find every caller of the engine.
rg -nP --type=py -C3 '\bapply_change_set\s*\(' apiRepository: Agenta-AI/agenta
Length of output: 1991
Validate set and remove before treating the delta as legacy.
_classify allows legacy forms whose values are not an object/list. Later code calls methods that require the expected types, either raising AttributeError for invalid input or interpreting the value incorrectly, such as iterating a string remove entry one character at a time. Raise Reason.INVALID_DELTA for non-object delta["set"] and for non-string / empty delta["remove"] entries before apply_change_set proceeds.
There was a problem hiding this comment.
Acknowledged, fix queued. Confirmed: _classify only checks delta.get(name) is not None for the legacy fields, with no type check. A non-dict set reaches deep_merge's patch.items() and raises AttributeError instead of INVALID_DELTA; a non-list/string remove would be iterated character by character rather than refused.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 40 minutes and 32 seconds before sending another message.
There was a problem hiding this comment.
Acknowledged, fix queued. Confirmed: _classify only checks for unknown top-level delta fields and legacy/ordered exclusivity, no type check on delta["set"] or delta["remove"] entries, still true at the tip. A non-dict set value reaches deep_merge's .items() call (AttributeError), and a delta["remove"] that is a bare string rather than a list of strings gets iterated character by character as individual paths.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 37 minutes and 51 seconds before sending another message.
829b266 to
f07fa00
Compare
df8b834 to
d047462
Compare
d047462 to
e02e5e3
Compare
dbc47ba to
b364fbf
Compare
e02e5e3 to
1ee0a02
Compare
bc0e91a to
06525cc
Compare
84a9bb8 to
24018cb
Compare
06525cc to
521cdc4
Compare
24018cb to
8099b56
Compare
The pure function behind ordered configuration edits, per docs/design/agent-config-editing/contracts/change-set.md: seven operations, structured targets with the list selector, exact-first matching with per-class length-preserving tolerance, the @ag.file marker refusal, three-tier unique-name rules, split error codes with a next-step on every retryable one, and the commit scope policy. Engine only; the commit endpoint wires it in the next slice.
…is the standard and the package stays consistent
…e (final review F11)
…copy (CodeRabbit)
…very-teaching errors (live session hardening)
…any walk touches the delta; instructive refusals for match_mode and unkeyed lists; typed legacy fields; nearest-lines cost bound; contract doc updated
…case lives under invalid_operation_shape
…nly, never stored, silently stripped with a warning
521cdc4 to
26f59ea
Compare
8099b56 to
cf6316c
Compare
dosubot
Bot
added
size:XXL
Railway Preview Environment
|
feat(api): add the change-set engine for ordered config edits
1 participant
Context
An agent that wants to change one line of its own instructions has to send its whole configuration back. That is how the commit endpoint works today:
delta.setis deep-merged, and any list it names is replaced wholesale. So changing one skill means resending every skill, and the model reliably drops something on the way. It also means every edit carries the full tree through the context window.This PR adds the engine that makes a small edit expressible. It is pure: plain dicts in, a plain result out, no database, no pydantic, no I/O. Nothing calls it yet. The wrapper that does arrives in the next lane.
Changes
Seven operations, applied in array order, all or nothing:
set,merge,remove,edit_text,add_item,replace_item,remove_item. A target is an array of segments from the configuration root. A string segment names an object field. An object segment names one entry of a list and stands in place of that list's name.Before, changing one skill's body:
{"delta": {"set": {"parameters": {"agent": {"skills": [ {"name": "release-qa", "body": "the new text"}, {"name": "triage", "body": "...resend this whole entry or lose it..."} ]}}}}}After:
{"delta": {"operations": [{ "operation": "set", "target": ["parameters", "agent", {"list": "skills", "key": "release-qa"}, "body"], "value": "the new text" }]}}edit_textgoes further and replaces an exact substring, so a long instruction document can be edited without resending it. An anchor must appear exactly once, counted with overlap, or the operation fails rather than guessing.Match tolerance follows what the text is. Prose fields (
agents_md,body,description) match exact first, then retry with normalized quotes, dashes, and whitespace, and the response reports that normalization was used. Script and file contents (content,script) match exact only, because bytes are meaning there.The engine also owns the refusals. Every operation's target is checked against a scope policy before anything is applied, and the check is on the RESULT rather than the named target: an operation that writes an ancestor object whose value would change a platform-owned path is refused too. That closed a live bypass found in UI testing, where an agent switched its own harness by writing the object above it. A result that would hold an unresolved
@ag.filemarker is rejected, since the runner replaces every marker before the API sees the call and one that survives means the runner did not run.Duplicate names are enforced in three tiers. An item operation must leave its collection clean, a branch write must not add a duplicate, and an untouched collection only warns. That last tier is what keeps existing configurations editable.
Nested collections carry their parent's key in their identity, so
skills[alpha].filesandskills[beta].filesare two collections. Without that they collapse onto one path and the last skill in the list answers for every other one, which makes the outcome depend on sibling order in both directions: a clean edit gets rejected because an untouched sibling holds duplicates, and a real duplicate written into a non-last skill is accepted in silence.Every reason code is stable and machine readable, and every retryable one carries a sentence naming the next action. The usability spike measured this as the difference between a model that recovers and one that dead-ends.
Tests / notes
api/oss/tests/pytest/unit/workflows/test_change_set.py, 188 tests. It is written againstcontracts/change-set.md, and each class names the contract section it pins.setandremovefold is pinned against an independent reference implementation written out in the test file, so the engine's legacy arm cannot drift from what the service did before._check_unique_namesand the touched-path bookkeeping around it. It is the part with the most cases and the least obvious failure mode.This targets
agent-config-editing-s4and is part of the agent-config-editing stack. Read the stack bottom up.