From e793efaeef65d40cd00cec7afcc2ffee5cb74b17 Mon Sep 17 00:00:00 2001 From: jonahgabriel Date: Sat, 3 Oct 2026 00:57:49 -0400 Subject: [PATCH 1/2] fix(OMN-20291): code-edit loop stops re-reading, keeps its own absolute paths, applies indent-slipped edits, caps bulk edits by files changed The OMN-20291 replay of 26 crush tasks left the node failing 8. Four node-side causes are fixed here: - Read loop (a16a3138, e3a2c923: 40 turns, 0 and 1 edits). The prompt sat at its 90k-character cap and held only the last one to three turns, and one turn's reads (8 to 12 views of up to 16 KB) exceeded the whole history, so even the newest turn was cut. Now every earlier turn keeps its action lines, the newest turns keep their output, a re-viewed window points at the later turn, a view of a file changed later is marked stale, one turn's reads total at most 30,000 characters, and after three turns that read and change no file the next turn's reads are refused, saying so in its prompt. History turns are structured, and the resume block (OMN-20391) stores them as JSON. - Absolute paths inside the worktree (58744096). The loop prints check argv with the worktree's absolute PYTHONPATH; the model searched that path and was refused as leaving the worktree. Such a path is now used and recorded as its worktree-relative form; any other absolute path is still refused. - Edit anchors (ea3ee264). The view prints ' 96| text'; the model copied the separator's blank into old_string, on every line or on the first line only. An old_string that matches exactly one place up to that shift is applied, with new_string moved back the same way. An edit whose result is already in the file is reported applied and writes nothing. - Bulk edit cap (8e1b5f72, OMN-20392 AC2 loop 7ea45ffe). replace_in_files refused a glob matching more node contracts than MAX_BULK_FILES before skipping the files without old_string. The cap now counts the files the glob would edit. Onex-Lane: code-edit-node-compare Onex-Session: 4380aed584814eab8ef70338fafb073d --- .../__init__.py | 6 + ...andler_delegated_code_edit_orchestrator.py | 346 +++++++++++++-- .../handlers/turn_protocol.py | 207 ++++++++- .../models/model_delegated_code_edit.py | 10 + ...andler_delegated_code_edit_orchestrator.py | 406 +++++++++++++++++- 5 files changed, 903 insertions(+), 72 deletions(-) diff --git a/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/__init__.py b/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/__init__.py index f6284626cd..0a945b0c23 100644 --- a/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/__init__.py +++ b/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/__init__.py @@ -18,8 +18,11 @@ from omnimarket.nodes.node_delegated_code_edit_orchestrator.handlers.turn_protocol import ( RESPONSE_CONTRACT, TOOL_SCHEMAS, + HistoryAction, + HistoryTurn, build_turn_prompt, parse_turn_reply, + render_history, ) from omnimarket.nodes.node_delegated_code_edit_orchestrator.models.model_delegated_code_edit import ( MAX_ERROR_CHARS, @@ -53,6 +56,8 @@ class NodeDelegatedCodeEditOrchestrator(HandlerDelegatedCodeEditOrchestrator): "EnumCodeEditStatus", "EnumCodeEditTool", "HandlerDelegatedCodeEditOrchestrator", + "HistoryAction", + "HistoryTurn", "LoopReceiptExistsError", "ModelCheckResult", "ModelCodeEditAction", @@ -69,5 +74,6 @@ class NodeDelegatedCodeEditOrchestrator(HandlerDelegatedCodeEditOrchestrator): "build_turn_prompt", "normalise_path", "parse_turn_reply", + "render_history", "writable", ] diff --git a/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/handlers/handler_delegated_code_edit_orchestrator.py b/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/handlers/handler_delegated_code_edit_orchestrator.py index 8c97bf468c..777cd3cfea 100644 --- a/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/handlers/handler_delegated_code_edit_orchestrator.py +++ b/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/handlers/handler_delegated_code_edit_orchestrator.py @@ -11,9 +11,26 @@ CLAIM -> [TURN -> APPLY(actions) -> (FINISH -> CHECKS)]* -> DIFF -> SCORE -> RECEIPT * At most ``max_turns`` turns (the tool_use rubric's budget caps it at 40). -* Writes are confined: a write, edit or replace_in_files whose path is absolute, - climbs out with ``..``, touches ``.git``, or matches no writable glob is - refused and the refusal is fed back. Nothing is written for it. +* One turn's reads (view, grep, ls) show at most ``MAX_READ_CHARS_PER_TURN`` + characters together: a view is shortened to what is left, and a read with + less than ``MIN_READ_CHARS`` left is not run and says so. A turn's reads then + fit the history whole (OMN-20291). +* After ``MAX_READ_ONLY_TURNS`` turns in a row that read and change no file, + the next turn's prompt says its reads are refused, and they are: it may + write, edit, format, run checks or finish. A file change ends the streak; a + refused turn that still changes nothing allows one more reading turn. +* Writes are confined: a write, edit or replace_in_files whose path leaves the + worktree (an absolute path outside it, or one that climbs out with ``..``), + touches ``.git``, or matches no writable glob is refused and the refusal is + fed back. Nothing is written for it. An absolute path inside the worktree is + used, and recorded, as its worktree-relative form: the loop itself prints + such paths in check commands (OMN-20291). +* An edit whose old_string is absent is retried once with each line's leading + and trailing whitespace free, and applied only when that finds exactly one + place whose lines after the first all moved by the same indentation (the + first may move on its own); new_string moves back the same way. An edit + whose result is already in the file is reported applied and writes nothing + (OMN-20291). * ``run_check`` runs only a check the request declares, by name; an undeclared name is refused. There is no shell tool. * ``finish`` runs every declared check. All passing ends the loop @@ -48,14 +65,19 @@ from omnimarket.nodes.node_delegated_code_edit_orchestrator.handlers.turn_protocol import ( RESPONSE_CONTRACT, TOOL_SCHEMAS, + HistoryAction, + HistoryTurn, build_turn_prompt, ) from omnimarket.nodes.node_delegated_code_edit_orchestrator.models.model_delegated_code_edit import ( MAX_BULK_FILES, MAX_OBSERVATION_BYTES, + MAX_READ_CHARS_PER_TURN, + MAX_READ_ONLY_TURNS, MAX_VIEW_BYTES, MAX_VIEW_WINDOW_BYTES, MAX_WRITE_BYTES, + MIN_READ_CHARS, VIEW_WINDOW_LINES, WRITING_TOOLS, EnumCodeEditStatus, @@ -81,10 +103,24 @@ PROMPT_BYTES = 120_000 -def normalise_path(path: str) -> str | None: - """The worktree-relative form of ``path``, or None when it leaves the worktree.""" - if not path or path.startswith("/") or "\x00" in path: +def normalise_path(path: str, *, root: str = "") -> str | None: + """The worktree-relative form of ``path``, or None when it leaves the worktree. + + An absolute path is accepted only under ``root``, the worktree's absolute + path, and comes back relative to it. + """ + if not path or "\x00" in path: return None + if path.startswith("/"): + base = posixpath.normpath(root) if root.startswith("/") else "" + norm = posixpath.normpath(path) + if not base or base == "/": + return None + if norm == base: + return "." + if not norm.startswith(base + "/"): + return None + path = norm[len(base) + 1 :] norm = posixpath.normpath(path) if norm == ".." or norm.startswith("../"): return None @@ -153,8 +189,11 @@ def _cap(text: str, limit: int = MAX_OBSERVATION_BYTES) -> str: _LINE_PREFIX = re.compile(r"^ *\d+\| ?", re.M) -def view_window(text: str, path: str, offset: int) -> str: - """One page of a file with line numbers, and how to see the rest.""" +def view_window( + text: str, path: str, offset: int, *, limit: int = MAX_VIEW_WINDOW_BYTES +) -> str: + """One page of a file with line numbers, at most ``limit`` characters of + rows, and how to see the rest.""" lines = text.splitlines() total = len(lines) start = max(offset, 1) @@ -165,7 +204,7 @@ def view_window(text: str, path: str, offset: int) -> str: end = start - 1 for number in range(start, min(total, start - 1 + VIEW_WINDOW_LINES) + 1): row = f"{number:>5}| {lines[number - 1]}" - if size + len(row) + 1 > MAX_VIEW_WINDOW_BYTES: + if size + len(row) + 1 > min(limit, MAX_VIEW_WINDOW_BYTES): break shown.append(row) size += len(row) + 1 @@ -189,6 +228,91 @@ def _edit_hint(current: str, old: str) -> str: ) +_READING_TOOLS = frozenset( + {EnumCodeEditTool.VIEW, EnumCodeEditTool.GREP, EnumCodeEditTool.LS} +) + + +def _lead(text: str) -> str: + """The blanks a line starts with.""" + return text[: len(text) - len(text.lstrip(" \t"))] + + +def _loose_pattern(old: str) -> re.Pattern[str] | None: + """old_string with each line's leading and trailing blanks free.""" + lines = old.split("\n") + if not any(line.strip() for line in lines): + return None + parts = [r"[ \t]*" + re.escape(line.strip(" \t")) for line in lines] + if not lines[-1].strip(): + # old ends with a newline: the next line's indentation is not part of it. + parts[-1] = "" + return re.compile(r"[ \t]*\n".join(parts)) + + +def _shift(line: str, by: int) -> str | None: + """``line`` moved left by ``by`` spaces (right when negative), or None when + it does not start with the blanks to remove.""" + if not line.strip() or by == 0: + return line + if by > 0: + return line[by:] if line.startswith(" " * by) else None + return " " * -by + line + + +def _indent_shift_edit(current: str, old: str, new: str) -> str | None: + """The file with ``old`` replaced by ``new`` when ``old`` matches exactly one + place up to indentation, and its lines after the first all moved by the same + number of spaces (the first line may have moved on its own); ``new`` is moved + back the same way. Otherwise None. + + A local model copies view rows `` 96| text`` with one blank too many or + too few, on every line or on the first line only; the content is right, + only the indentation shifted. + """ + pattern = _loose_pattern(old) + if pattern is None: + return None + matches = list(pattern.finditer(current)) + if len(matches) != 1: + return None + match = matches[0] + found = current[match.start() : match.end()].split("\n") + wanted = old.split("\n") + at_line_start = match.start() == 0 or current[match.start() - 1] == "\n" + first: int | None = None + rest: set[int] = set() + for number, (have, want) in enumerate(zip(found, wanted, strict=True)): + if not want.strip() or (number == 0 and not at_line_start): + continue + have_lead, want_lead = _lead(have), _lead(want) + if "\t" in have_lead + want_lead: + if have_lead != want_lead: + return None + shift = 0 + else: + shift = len(want_lead) - len(have_lead) + if number == 0: + first = shift + else: + rest.add(shift) + if len(rest) > 1: + return None + later = next(iter(rest)) if rest else (first or 0) + moved: list[str] = [] + for number, line in enumerate(new.split("\n")): + shifted = _shift(line, first if number == 0 and first is not None else later) + if shifted is None: + return None + moved.append(shifted) + replacement = "\n".join(moved) + if not at_line_start or not old[:1].isspace(): + # The match swallowed the file's own leading blanks of a line old did + # not indent; keep them. + replacement = _lead(current[match.start() :]) + replacement.lstrip(" \t") + return current[: match.start()] + replacement + current[match.end() :] + + @dataclass class _Call: """One applied action, as the transcript records it.""" @@ -205,7 +329,7 @@ class _State: paths: tuple[str, ...] = () replies: list[ModelTurnReply] = field(default_factory=list) calls: list[_Call] = field(default_factory=list) - history: list[str] = field(default_factory=list) + history: list[HistoryTurn] = field(default_factory=list) turns: list[dict[str, object]] = field(default_factory=list) refusals: int = 0 last_checks: tuple[ModelCheckResult, ...] = () @@ -215,6 +339,7 @@ class _State: good_history_len: int = 0 superseded: list[dict[str, object]] = field(default_factory=list) resumes: int = 0 + read_only_streak: int = 0 def record_good(self, turn: int) -> None: """Snapshot the history boundary after a received reply was processed.""" @@ -386,7 +511,10 @@ def _seed(receipt: dict[str, object]) -> _State: result = cast(dict[str, object], receipt["result"]) state = _State( turns=[t for t in turns if cast(int, t["turn"]) <= good_turn], - history=list(cast(list[str], block["history"])), + history=[ + HistoryTurn.from_json(entry) + for entry in cast(list[object], block["history"]) + ], refusals=cast(int, result["refusals"]), summary=cast(str, result["summary"]), last_checks=tuple( @@ -462,14 +590,16 @@ def _resume_block( f"{len(state.turns)} turns used of {request.max_turns}; " "only a delegate_failed loop resumes" ) - history: list[str] = [] + history: list[dict[str, object]] = [] size = 0 if resumable: - for block in reversed(state.history[: state.good_history_len]): - if size + len(block) > 120_000: + for entry in reversed(state.history[: state.good_history_len]): + kept = entry.to_json() + length = len(json.dumps(kept)) + if size + length > 120_000: break - history.append(block) - size += len(block) + history.append(kept) + size += length history.reverse() return { "resumable": resumable, @@ -515,7 +645,14 @@ def _turns( index = relevant_first(request, [path for path, _ in manifest]) failed_delegates = 0 for turn in range(start_turn, request.max_turns + 1): - prompt = self._prompt(request, index, context, state, turn) + prompt = self._prompt( + request, + index, + context, + state, + turn, + reads_paused=state.read_only_streak >= MAX_READ_ONLY_TURNS, + ) reply = self._ports.delegate(request, prompt, RESPONSE_CONTRACT, turn) state.replies.append(reply) record: dict[str, object] = { @@ -533,7 +670,11 @@ def _turns( if not reply.ok and not reply.raw_text: failed_delegates += 1 state.history.append( - f"TURN {turn}: the delegate run failed: {reply.invalid_reason[:300]}\n" + HistoryTurn( + turn, + message=f"TURN {turn}: the delegate run failed: " + f"{reply.invalid_reason[:300]}\n", + ) ) if failed_delegates >= 2: raise _TerminalError( @@ -547,8 +688,11 @@ def _turns( if not reply.ok or not reply.actions: reason = reply.invalid_reason or "the reply carried no actions" state.history.append( - f"TURN {turn}: your reply was unusable: {reason[:300]}. " - "Reply with one JSON object with an actions list.\n" + HistoryTurn( + turn, + message=f"TURN {turn}: your reply was unusable: {reason[:300]}. " + "Reply with one JSON object with an actions list.\n", + ) ) state.record_good(turn) continue @@ -560,11 +704,14 @@ def _turns( if all(c.status == "passed" for c in checks): return EnumCodeEditStatus.ACCEPTED state.history.append( - "FINISH REFUSED: these checks did not pass:\n" - + "".join( - f"- {c.name}: {c.status}\n{_cap(c.output_tail, 2_000)}\n" - for c in checks - if c.status != "passed" + HistoryTurn( + turn, + message="FINISH REFUSED: these checks did not pass:\n" + + "".join( + f"- {c.name}: {c.status}\n{_cap(c.output_tail, 2_000)}\n" + for c in checks + if c.status != "passed" + ), ) ) self._progress_or_stop(request, checks, state) @@ -585,6 +732,8 @@ def _prompt( context: tuple[tuple[str, str], ...], state: _State, turn: int, + *, + reads_paused: bool = False, ) -> str: """The turn prompt, shrunk until it fits one argv word.""" limit = PROMPT_CHARS @@ -596,6 +745,7 @@ def _prompt( state.history, turn, max_chars=limit, + reads_paused=reads_paused, ) if len(prompt.encode("utf-8")) <= PROMPT_BYTES or limit <= 20_000: return prompt @@ -609,18 +759,45 @@ def _apply_turn( record: dict[str, object], state: _State, ) -> bool: - block = [f"TURN {turn}\n"] + shown: list[HistoryAction] = [] finished = False actions_log: list[dict[str, object]] = [] - for number, action in enumerate(reply.actions, start=1): + read_left = MAX_READ_CHARS_PER_TURN + paused = state.read_only_streak >= MAX_READ_ONLY_TURNS + read_any = changed_any = False + for number, given in enumerate(reply.actions, start=1): + action = self._rebased(request, given) + reads = action.tool in _READING_TOOLS if action.tool == EnumCodeEditTool.FINISH: finished = True state.summary = action.summary observation = ModelObservation( ok=True, output="finish: checks run next" ) + elif reads and paused: + observation = ModelObservation( + ok=False, + output=f"not read: {state.read_only_streak} turns in a row read " + "and change no file, so this turn's reads are refused; write or " + "edit with what HISTORY shows", + ) + elif reads and read_left < MIN_READ_CHARS: + observation = ModelObservation( + ok=False, + output=f"not read: this turn's reads already show " + f"{MAX_READ_CHARS_PER_TURN - read_left} characters, the most one " + "turn may; edit what you have read, then read more next turn", + ) else: - observation = self._apply(request, action, state) + observation = self._apply(request, action, state, read_left) + if reads and observation.ok: + read_left -= len(observation.output) + read_any = read_any or reads + changed_any = changed_any or ( + observation.ok + and action.tool in WRITING_TOOLS + and not observation.output.startswith("unchanged") + ) arguments = { key: value for key, value in action.model_dump( @@ -652,19 +829,57 @@ def _apply_turn( for k, v in arguments.items() if k not in ("content", "old_string", "new_string") ) - block.append( - f"> {action.tool.value}({shown_args}) -> " - f"{'ok' if observation.ok else 'REFUSED/ERROR'}\n{observation.output}\n" + path = normalise_path(action.target) if action.target else None + shown.append( + HistoryAction( + header=f"> {action.tool.value}({shown_args}) -> " + f"{'ok' if observation.ok else 'REFUSED/ERROR'}", + output=observation.output, + view_key=(path, max(action.offset, 1)) + if observation.ok + and path is not None + and action.tool == EnumCodeEditTool.VIEW + else None, + changed_path=path + if observation.ok + and path is not None + and action.tool in WRITING_TOOLS + and not observation.output.startswith("unchanged") + else "", + ) ) record["actions"] = actions_log - state.history.append("".join(block)) + state.history.append(HistoryTurn(turn, actions=tuple(shown))) + if changed_any: + state.read_only_streak = 0 + elif paused: + # One more reading turn, then the pause again. + state.read_only_streak = MAX_READ_ONLY_TURNS - 1 + elif read_any: + state.read_only_streak += 1 return finished + @staticmethod + def _rebased( + request: ModelDelegatedCodeEditRequest, action: ModelCodeEditAction + ) -> ModelCodeEditAction: + """The action with an absolute path inside the worktree made relative, so + it is applied, shown and recorded as the path it names.""" + updates: dict[str, str] = {} + for name in ("path", "file_path"): + value = getattr(action, name) + if value.startswith("/"): + rel = normalise_path(value, root=request.workspace_root) + if rel is not None: + updates[name] = rel + return action.model_copy(update=updates) if updates else action + def _apply( self, request: ModelDelegatedCodeEditRequest, action: ModelCodeEditAction, state: _State, + read_left: int = MAX_READ_CHARS_PER_TURN, ) -> ModelObservation: tool = action.tool if tool == EnumCodeEditTool.REPLACE_IN_FILES: @@ -704,16 +919,24 @@ def _apply( if tool == EnumCodeEditTool.VIEW: text = self._ports.read_file(request, path) return ModelObservation( - ok=True, output=view_window(text, path, action.offset) + ok=True, + output=view_window(text, path, action.offset, limit=read_left), ) if tool == EnumCodeEditTool.LS: return ModelObservation( - ok=True, output=_cap(self._ports.list_dir(request, path)) + ok=True, + output=_cap( + self._ports.list_dir(request, path), + min(MAX_OBSERVATION_BYTES, read_left), + ), ) if tool == EnumCodeEditTool.GREP: return ModelObservation( ok=True, - output=_cap(self._ports.grep(request, action.pattern, path)), + output=_cap( + self._ports.grep(request, action.pattern, path), + min(MAX_OBSERVATION_BYTES, read_left), + ), ) if tool == EnumCodeEditTool.WRITE: if len(action.content.encode()) > MAX_WRITE_BYTES: @@ -738,14 +961,37 @@ def _apply( old_string = _LINE_PREFIX.sub("", old_string) new_string = _LINE_PREFIX.sub("", new_string) occurrences = current.count(old_string) - if occurrences != 1: + if ( + new_string + and current.count(new_string) == 1 + and ( + occurrences == 0 + or ( + old_string in new_string + and occurrences == new_string.count(old_string) + ) + ) + ): + # The file already holds this edit's result: a model re-sending + # an edit it applied in an earlier turn. Applying an insertion + # again would duplicate it; reporting it failed is untrue. + return ModelObservation( + ok=True, + output=f"unchanged {path}: new_string is already there once " + "(this edit was applied before)", + ) + updated: str | None = None + if occurrences == 0: + updated = _indent_shift_edit(current, old_string, new_string) + if updated is None and occurrences != 1: hint = _edit_hint(current, old_string) if occurrences == 0 else "" return ModelObservation( ok=False, output=f"edit failed: old_string occurs {occurrences} times in " f"{path}; it must occur exactly once.{hint}", ) - updated = current.replace(old_string, new_string, 1) + if updated is None: + updated = current.replace(old_string, new_string, 1) self._ports.write_file(request, path, updated) return ModelObservation(ok=True, output=f"edited {path}") except WorkspacePathError as exc: @@ -762,10 +1008,14 @@ def _replace_in_files( A glob is a scope, not a list: it expands to the manifest files that are writable, a matching file outside the writable globs is counted and skipped, and a file without old_string is not a failure. A named file - that is refused or lacks old_string is. + that is refused or lacks old_string is. The file cap counts the files a + glob would edit, those holding old_string, not every file it matches + (OMN-20291: ``nodes/*/contract.yaml`` matches more than the cap). """ by_glob = not action.file_paths outside = 0 + unmatched = 0 + scope = 0 if not by_glob: targets = list( dict.fromkeys( @@ -777,7 +1027,19 @@ def _replace_in_files( matched = sorted(path for path in state.paths if pattern.match(path)) targets = [path for path in matched if writable(request, path)] outside = len(matched) - len(targets) - if not targets or len(targets) > MAX_BULK_FILES: + scope = len(targets) + holding: list[str] = [] + for name in targets: + try: + if action.old_string not in self._ports.read_file(request, name): + unmatched += 1 + continue + except WorkspacePathError: + pass # reported per file below + holding.append(name) + if scope: + targets = holding + if (not targets and not (by_glob and scope)) or len(targets) > MAX_BULK_FILES: state.refusals += 1 return ModelObservation( ok=False, @@ -787,7 +1049,6 @@ def _replace_in_files( ) failures: list[str] = [] edited: list[str] = [] - unmatched = 0 for raw in targets: path = normalise_path(raw) if path is None: @@ -820,7 +1081,10 @@ def _replace_in_files( edited.append(f"edited {path} ({occurrences}x)") if by_glob and not edited and not failures: failures.append("FAILED glob: no matched file contains old_string") - header = f"replace_in_files: {len(edited)} edited, {len(failures)} failed of {len(targets)}" + header = ( + f"replace_in_files: {len(edited)} edited, {len(failures)} failed " + f"of {scope if by_glob else len(targets)}" + ) if by_glob: header += f"; {unmatched} without old_string, {outside} matched outside the writable globs" return ModelObservation( diff --git a/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/handlers/turn_protocol.py b/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/handlers/turn_protocol.py index 087f34d7be..8d09521012 100644 --- a/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/handlers/turn_protocol.py +++ b/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/handlers/turn_protocol.py @@ -12,6 +12,9 @@ cannot. * ``build_turn_prompt`` renders one turn's prompt from the request, the file index and the history. Same inputs, same bytes. +* ``render_history`` fits the history to its budget (OMN-20291): every earlier + turn keeps its action lines, the newest turns keep their full output, and a + view window shown again later is not repeated. """ from __future__ import annotations @@ -19,12 +22,16 @@ import json import re from collections.abc import Sequence +from dataclasses import dataclass +from typing import cast from pydantic import ValidationError from omnimarket.nodes.node_delegated_code_edit_orchestrator.models.model_delegated_code_edit import ( MAX_ACTIONS_PER_TURN, MAX_BULK_FILES, + MAX_READ_CHARS_PER_TURN, + MAX_READ_ONLY_TURNS, EnumCodeEditTool, ModelCodeEditAction, ModelDelegatedCodeEditRequest, @@ -246,16 +253,177 @@ def _cap(text: str, limit: int) -> str: return text[:limit] + f"\n... [{len(text) - limit} more characters cut]\n" +@dataclass(frozen=True) +class HistoryAction: + """One applied action as the history shows it.""" + + #: ``> view(path='a.py') -> ok`` + header: str + output: str + #: (path, first line) of a view that succeeded. + view_key: tuple[str, int] | None = None + #: The path a write, edit or format that succeeded changed. + changed_path: str = "" + + +@dataclass(frozen=True) +class HistoryTurn: + """One earlier turn: its applied actions, or one message about it (a failed + delegate run, an unusable reply, a refused finish).""" + + turn: int + actions: tuple[HistoryAction, ...] = () + message: str = "" + + def to_json(self) -> dict[str, object]: + """The turn as a loop receipt's resume block keeps it.""" + return { + "turn": self.turn, + "message": self.message, + "actions": [ + { + "header": a.header, + "output": a.output, + "view_key": list(a.view_key) if a.view_key else None, + "changed_path": a.changed_path, + } + for a in self.actions + ], + } + + @classmethod + def from_json(cls, value: object) -> HistoryTurn: + """A turn from a resume block; a plain string (a receipt written before + turns were structured) is one message.""" + if isinstance(value, str): + return cls(0, message=value) + row = cast("dict[str, object]", value) + actions: list[HistoryAction] = [] + for raw in cast("list[dict[str, object]]", row.get("actions", [])): + key = raw.get("view_key") + pair = cast("list[object]", key) if isinstance(key, list) else None + actions.append( + HistoryAction( + header=str(raw["header"]), + output=str(raw["output"]), + view_key=(str(pair[0]), int(str(pair[1]))) if pair else None, + changed_path=str(raw.get("changed_path", "")), + ) + ) + return cls( + int(str(row["turn"])), + actions=tuple(actions), + message=str(row.get("message", "")), + ) + + +#: Characters of one action's output an elided turn still shows. +_BRIEF_OUTPUT_CHARS = 160 + + +def _brief(output: str, view: bool) -> str: + first = output.split("\n", 1)[0] + whole = first == output + if view and first.startswith("[") and not whole: + return first + " [content elided to fit; view again only what you will edit]" + if len(first) > _BRIEF_OUTPUT_CHARS: + return first[:_BRIEF_OUTPUT_CHARS] + " ... [output elided to fit]" + return first if whole else first + " [output elided to fit]" + + +def _render_turns(history: Sequence[HistoryTurn]) -> list[tuple[str, str]]: + """(full, brief) renderings of each turn, oldest first. + + A view window shown again in a later turn points at that turn instead of + repeating its lines, and a view of a file a later action changed says so, + so an anchor is not copied from content that is no longer there. + """ + shown_later: dict[tuple[str, int], int] = {} + changed_later: dict[str, int] = {} + rendered: list[tuple[str, str]] = [] + for entry in reversed(history): + if not entry.actions: + text = ( + entry.message if entry.message.endswith("\n") else entry.message + "\n" + ) + rendered.append((text, _cap(text, 400))) + continue + full: list[str] = [] + brief: list[str] = [] + for action in reversed(entry.actions): + body = action.output + if action.view_key is not None: + later = shown_later.get(action.view_key) + if later is not None: + body = ( + body.split("\n", 1)[0] + + f" [same window shown again in turn {later}]" + ) + elif action.view_key[0] in changed_later: + window, _, lines = body.partition("\n") + body = ( + f"{window} [note: {action.view_key[0]} changed in turn " + f"{changed_later[action.view_key[0]]} after this view]\n" + + lines + ) + shown_later.setdefault(action.view_key, entry.turn) + if action.changed_path: + changed_later.setdefault(action.changed_path, entry.turn) + full.append(f"{action.header}\n{body}\n") + brief.append( + f"{action.header}\n{_brief(body, action.view_key is not None)}\n" + ) + head = f"TURN {entry.turn}\n" + rendered.append( + (head + "".join(reversed(full)), head + "".join(reversed(brief))) + ) + rendered.reverse() + return rendered + + +def render_history(history: Sequence[HistoryTurn], budget: int) -> str: + """The history in at most about ``budget`` characters. + + Every turn first gets its brief form (action lines and one line of each + output), oldest dropped only when even those do not fit; then the newest + turns are shown in full while the budget lasts. A newest turn too large to + show whole is shown cut, never dropped: its outcomes are the model's only + memory of them. + """ + turns = _render_turns(history) + chosen = [brief for _, brief in turns] + total = sum(len(text) for text in chosen) + first = 0 + while first < len(chosen) and total > budget: + total -= len(chosen[first]) + first += 1 + for index in range(len(turns) - 1, first - 1, -1): + extra = len(turns[index][0]) - len(chosen[index]) + if total + extra <= budget: + chosen[index] = turns[index][0] + total += extra + continue + if index == len(turns) - 1: + room = budget - (total - len(chosen[index])) + if room > _MIN_TURN_CHARS: + chosen[index] = _cap(turns[index][0], room) + break + marker = "[earlier turns cut to fit]\n" if first else "" + return marker + "".join(chosen[first:]) + + def build_turn_prompt( request: ModelDelegatedCodeEditRequest, file_index: Sequence[str], context: Sequence[tuple[str, str]], - history: Sequence[str], + history: Sequence[HistoryTurn], turn: int, *, max_chars: int = 90_000, + reads_paused: bool = False, ) -> str: - """One turn's prompt. ``history`` holds the rendered previous turns, oldest first.""" + """One turn's prompt. ``history`` holds the previous turns, oldest first. + ``reads_paused`` says this turn's reads are refused.""" checks = "\n".join(f"- {c.name}: {' '.join(c.argv)}" for c in request.checks) globs = ", ".join(request.writable_globs) tools = "\n".join( @@ -267,28 +435,32 @@ def build_turn_prompt( "through tools. Reply with ONE JSON object and nothing else:\n" '{"note": "", "actions": [{"tool": "", ...}, ...]}\n' f"At most {MAX_ACTIONS_PER_TURN} actions per turn. Paths are relative " - "to the worktree root. Read before you edit. Results come back next turn.\n\n" + "to the worktree root. Read before you edit. Results come back next turn.\n" + f"One turn's reads (view, grep, ls) show at most {MAX_READ_CHARS_PER_TURN} " + "characters together. HISTORY keeps every earlier turn, older turns " + "without their output: a view whose content is elided there was already " + "read. Do not re-read whole files; view the lines you will change and " + "edit them in the same or the next turn. After " + f"{MAX_READ_ONLY_TURNS} turns in a row that read and change no file, the " + "next turn's reads are refused.\n\n" f"TOOLS\n{tools}\n\n" f"WRITABLE (only these paths may be written): {globs}\n\n" f"CHECKS (run_check by name; finish runs all of them)\n{checks}\n\n" - f"TURN {turn} of {request.max_turns}. Call finish once the checks should pass.\n\n" + f"TURN {turn} of {request.max_turns}. Call finish once the checks should pass.\n" + + ( + f"THIS TURN: {MAX_READ_ONLY_TURNS} or more turns in a row read and changed " + "no file, so reads are refused this turn. Write or edit now with what " + "HISTORY shows; a view row reads 'NNNN| text', copy only the text.\n" + if reads_paused + else "" + ) + + "\n" f"TASK\n{request.task}\n\n" ) index = "FILES\n" + _cap("\n".join(file_index), 12_000) + "\n\n" shown = "".join(f"FILE {path}\n{_cap(body, 20_000)}\n\n" for path, body in context) budget = max_chars - len(head) - len(index) - len(shown) - kept: list[str] = [] - for block in reversed(history): - if budget - len(block) < 0: - # A turn whose results exceed what is left is shown cut, never - # dropped: its actions and outcomes are the model's only memory. - if budget > _MIN_TURN_CHARS: - kept.append(_cap(block, budget)) - kept.append("[earlier turns cut to fit]\n") - break - kept.append(block) - budget -= len(block) - past = "".join(reversed(kept)) + past = render_history(history, budget) if history else "" return head + index + shown + (f"HISTORY\n{past}" if past else "") @@ -299,6 +471,9 @@ def build_turn_prompt( "REQUIRED_ARGUMENTS", "RESPONSE_CONTRACT", "TOOL_SCHEMAS", + "HistoryAction", + "HistoryTurn", "build_turn_prompt", "parse_turn_reply", + "render_history", ] diff --git a/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/models/model_delegated_code_edit.py b/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/models/model_delegated_code_edit.py index 482c3ded6b..b98cbf80bc 100644 --- a/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/models/model_delegated_code_edit.py +++ b/src/omnimarket/nodes/node_delegated_code_edit_orchestrator/models/model_delegated_code_edit.py @@ -38,6 +38,16 @@ VIEW_WINDOW_LINES = 250 #: Bytes one view window may carry. MAX_VIEW_WINDOW_BYTES = 16_000 +#: Characters the reads (view, grep, ls) of one turn may show together. The +#: history holds about 66,000 characters, so a turn's reads fit it whole with +#: room for the turn before (OMN-20291). +MAX_READ_CHARS_PER_TURN = 30_000 +#: Below this many characters left, a turn's further reads are not run. +MIN_READ_CHARS = 2_000 +#: Turns in a row that read and change no file before the next turn's reads are +#: refused, so a model that cannot hold every file it wants to read writes with +#: what it has instead of reading to the turn cap (OMN-20291). +MAX_READ_ONLY_TURNS = 3 _CHECK_NAME = re.compile(r"^[a-z0-9][a-z0-9_-]{0,47}$") diff --git a/tests/nodes/node_delegated_code_edit_orchestrator/test_handler_delegated_code_edit_orchestrator.py b/tests/nodes/node_delegated_code_edit_orchestrator/test_handler_delegated_code_edit_orchestrator.py index c220f72e74..cc62e3058e 100644 --- a/tests/nodes/node_delegated_code_edit_orchestrator/test_handler_delegated_code_edit_orchestrator.py +++ b/tests/nodes/node_delegated_code_edit_orchestrator/test_handler_delegated_code_edit_orchestrator.py @@ -648,12 +648,18 @@ def test_offset_must_be_a_positive_integer() -> None: def test_a_turn_whose_results_exceed_the_history_budget_is_cut_not_dropped() -> None: from omnimarket.nodes.node_delegated_code_edit_orchestrator import ( + HistoryAction, + HistoryTurn, build_turn_prompt, ) request = _request() - huge = "TURN 2\n> view(path='src/m.py') -> ok\n" + "x" * 50_000 + "\n" - history = ["TURN 1\n> ls() -> ok\nsrc\n", huge] + history = [ + HistoryTurn(1, actions=(HistoryAction("> ls() -> ok", "src"),)), + HistoryTurn( + 2, actions=(HistoryAction("> view(path='src/m.py') -> ok", "x" * 50_000),) + ), + ] head_only = build_turn_prompt(request, ["src/m.py"], (), [], 3, max_chars=1) prompt = build_turn_prompt( request, ["src/m.py"], (), history, 3, max_chars=len(head_only) + 5_000 @@ -737,9 +743,8 @@ def test_delegate_failed_receipt_preserves_last_good_history() -> None: assert block["max_turns"] == request.max_turns assert block["reason"] == "resumable from turn 3 (4 turns left)" assert len(block["history"]) == 2 - assert block["history"][0].startswith("TURN 1\n") - assert block["history"][1].startswith("TURN 2\n") - assert "delegate run failed" not in "".join(block["history"]) + assert [entry["turn"] for entry in block["history"]] == [1, 2] + assert "delegate run failed" not in json.dumps(block["history"]) assert ( block["diff_sha256"] == hashlib.sha256(ports.diff(request).encode()).hexdigest() ) @@ -771,7 +776,13 @@ def test_resume_restores_history_and_archives_failed_attempts() -> None: assert result.local_tokens_in == 11 assert result.local_tokens_out == 3 assert ports.delegate_turns[-1] == 3 - assert all(block in ports.prompts[-1] for block in history) + assert history + assert all(f"TURN {entry['turn']}\n" in ports.prompts[-1] for entry in history) + assert all( + action["header"] in ports.prompts[-1] + for entry in history + for action in entry["actions"] + ) receipt = ports.receipts[request.correlation_id] assert receipt["resumes"] == 1 assert [t["turn"] for t in cast(list[dict[str, Any]], receipt["turns"])] == [ @@ -869,24 +880,30 @@ def test_unusable_received_reply_is_a_good_resume_turn() -> None: block = cast(dict[str, Any], ports.receipts[request.correlation_id]["resume"]) assert block["last_good_turn"] == 1 assert len(block["history"]) == 1 - assert "your reply was unusable" in block["history"][0] + assert "your reply was unusable" in block["history"][0]["message"] def test_resume_history_keeps_newest_whole_blocks_within_budget() -> None: - actions = tuple(_a("view", path="src/m.py") for _ in range(4)) + # Each turn changes a file, so its reads are never paused, and reads its + # bounded 30,000 characters; eight such turns exceed the 120,000 budget. + actions = ( + _a("write", file_path="src/n.py", content="n = 1\n"), + *(_a("view", path="src/m.py") for _ in range(4)), + ) dead = ModelTurnReply(run_id="dead", ok=False) ports = FakePorts( - [_reply(n, *actions) for n in range(1, 5)] + [dead, dead], + [_reply(n, *actions) for n in range(1, 9)] + [dead, dead], files={"src/m.py": "x" * 10_000}, ) - request = _request(max_turns=10) + request = _request(max_turns=12) HandlerDelegatedCodeEditOrchestrator(ports).run(request) block = cast(dict[str, Any], ports.receipts[request.correlation_id]["resume"]) - assert block["last_good_turn"] == 4 - assert sum(map(len, block["history"])) <= 120_000 - assert len(block["history"]) == 2 - assert block["history"][0].startswith("TURN 3\n") - assert block["history"][1].startswith("TURN 4\n") + assert block["last_good_turn"] == 8 + assert sum(len(json.dumps(entry)) for entry in block["history"]) <= 120_000 + kept = [entry["turn"] for entry in block["history"]] + # The newest turns, whole, oldest dropped first. + assert kept == list(range(9 - len(kept), 9)) + assert 1 <= len(kept) < 8 def test_repeated_resumes_carry_attempts_and_preserve_check_state() -> None: @@ -1263,3 +1280,362 @@ def test_replace_in_files_schemas_declare_arrays_and_keep_the_turn_limit() -> No "minItems": 1, "maxItems": MAX_BULK_FILES, } + + +# -- OMN-20291: the replay's node failure classes ------------------------------ + + +def _calls(ports: FakePorts) -> list[dict[str, Any]]: + return cast("list[dict[str, Any]]", ports.transcript["calls"]) + + +def test_an_absolute_path_inside_the_worktree_is_used_as_its_relative_form() -> None: + """Replay 58744096: the loop prints check argv with the worktree's absolute + PYTHONPATH; the model searched that path and was refused as leaving the + worktree, and the rubric scored the absolute path a phantom path.""" + ports = FakePorts( + [ + _reply( + 1, + _a("view", path="/work/tree/src/m.py"), + _a("grep", pattern="add", path="/work/tree/src"), + _a( + "edit", + file_path="/work/tree/src/m.py", + old_string="return 0", + new_string="return a + b", + ), + _a("view", path="/work/treeX/src/m.py"), + _a("finish", summary="s"), + ) + ], + check_passes=[True], + ) + result = HandlerDelegatedCodeEditOrchestrator(ports).run(_request(max_turns=1)) + assert ports.files["src/m.py"] == FIX + calls = _calls(ports) + assert [c["status"] for c in calls] == ["ok", "ok", "ok", "error", "ok"] + assert '"path": "src/m.py"' in calls[0]["arguments_json"] + assert '"path": "src"' in calls[1]["arguments_json"] + assert '"file_path": "src/m.py"' in calls[2]["arguments_json"] + # A sibling directory that merely shares the prefix still leaves the worktree. + assert "leaves the worktree" in calls[3]["output"] + assert result.refusals == 1 + + +def test_normalise_path_maps_an_absolute_path_under_the_root() -> None: + assert normalise_path("/work/tree/src/m.py", root="/work/tree") == "src/m.py" + assert normalise_path("/work/tree", root="/work/tree") == "." + assert normalise_path("/work/tree/../x", root="/work/tree") is None + assert normalise_path("/tmp/x.py", root="/work/tree") is None + assert normalise_path("/abs") is None + + +DOC = ( + "def f():\n" + ' """Docs.\n' + "\n" + " * Owned: a CLAIM owns a PR by its ticket\n" + " (OMN-1), and no CLAIM owns a PR by a dispatcher. A later TERMINAL ends it.\n" + ' """\n' +) + + +def test_edit_tolerates_a_uniform_indentation_shift() -> None: + """Replay ea3ee264: the view prints `` 96| text`` and the model copied one + space of the ``| `` separator into every line of old_string and new_string.""" + ports = FakePorts( + [ + _reply( + 1, + _a( + "edit", + file_path="src/m.py", + old_string=" (OMN-1), and no CLAIM owns a PR by a dispatcher. A later TERMINAL", + new_string=" (OMN-1), and no CLAIM owns a PR by a dispatcher,\n" + " nor by a shared ticket. A later TERMINAL", + ), + ) + ], + files={"src/m.py": DOC}, + check_passes=[True], + ) + HandlerDelegatedCodeEditOrchestrator(ports).run(_request(max_turns=1)) + assert ports.files["src/m.py"] == DOC.replace( + "a dispatcher. A later", + "a dispatcher,\n nor by a shared ticket. A later", + ) + assert [c["status"] for c in _calls(ports)] == ["ok"] + + +def test_a_multi_line_edit_tolerates_a_uniform_indentation_shift() -> None: + ports = FakePorts( + [ + _reply( + 1, + _a( + "edit", + file_path="src/m.py", + old_string=" * Owned: a CLAIM owns a PR by its ticket\n" + " (OMN-1), and no CLAIM", + new_string=" * Owned: a CLAIM owns a PR by its ticket\n" + " (OMN-2), and no CLAIM", + ), + ) + ], + files={"src/m.py": DOC}, + check_passes=[True], + ) + HandlerDelegatedCodeEditOrchestrator(ports).run(_request(max_turns=1)) + assert ports.files["src/m.py"] == DOC.replace("(OMN-1)", "(OMN-2)") + + +def test_an_indentation_tolerant_match_must_still_be_unique() -> None: + body = "def f():\n x = 1\n\ndef g():\n x = 1\n" + ports = FakePorts( + [ + _reply( + 1, + _a( + "edit", + file_path="src/m.py", + old_string="\tx = 1", + new_string="\tx = 2", + ), + ) + ], + files={"src/m.py": body}, + ) + HandlerDelegatedCodeEditOrchestrator(ports).run(_request(max_turns=1)) + assert ports.files["src/m.py"] == body + assert _calls(ports)[0]["status"] == "error" + + +def test_an_edit_already_applied_is_reported_applied_not_failed() -> None: + """Replay ea3ee264 turn 7: the model re-sent an edit it had applied in turn 3.""" + ports = FakePorts( + [ + _reply( + 1, + _a( + "edit", + file_path="src/m.py", + old_string="return 0", + new_string="return a + b", + ), + ), + _reply( + 2, + _a( + "edit", + file_path="src/m.py", + old_string="return 0", + new_string="return a + b", + ), + ), + ], + check_passes=[True], + ) + HandlerDelegatedCodeEditOrchestrator(ports).run(_request(max_turns=2)) + assert ports.files["src/m.py"] == FIX + calls = _calls(ports) + assert [c["status"] for c in calls] == ["ok", "ok"] + assert "already" in calls[1]["output"] + assert ports.writes == ["src/m.py"] + + +def test_an_insertion_already_applied_is_not_inserted_twice() -> None: + ports = FakePorts( + [ + _reply( + 1, + _a( + "edit", + file_path="src/m.py", + old_string="def add(a, b):", + new_string="def add(a, b):\n # sum", + ), + ), + _reply( + 2, + _a( + "edit", + file_path="src/m.py", + old_string="def add(a, b):", + new_string="def add(a, b):\n # sum", + ), + ), + ], + ) + HandlerDelegatedCodeEditOrchestrator(ports).run(_request(max_turns=2)) + assert ports.files["src/m.py"] == "def add(a, b):\n # sum\n return 0\n" + assert [c["status"] for c in _calls(ports)] == ["ok", "ok"] + + +def _views(n: int, path: str = "src/big.py") -> ModelTurnReply: + return _reply( + n, *(_a("view", path=path, offset=str(1 + 250 * k)) for k in range(4)) + ) + + +def test_every_earlier_turn_stays_in_the_prompt_when_its_output_cannot() -> None: + """Replays a16a3138 and e3a2c923: from turn 3 the prompt sat at its cap and + held only the last one to three turns, so the model forgot what it had read + and re-read the same files for 40 turns without editing.""" + big = "".join(f"line {i} " + "x" * 40 + "\n" for i in range(1, 1001)) + turns = [ + _views(1), + _reply(2, _a("grep", pattern="line 7", path="src")), + *(_views(n) for n in range(3, 9)), + ] + ports = FakePorts(turns, files={"src/m.py": "x\n", "src/big.py": big}) + HandlerDelegatedCodeEditOrchestrator(ports).run(_request(max_turns=9)) + last = ports.prompts[-1] + for n in range(1, 9): + assert f"TURN {n}\n" in last, n + assert "> grep(path='src', pattern='line 7') -> ok" in last + assert "[earlier turns cut to fit]" not in last + # The newest turn's views are shown in full; a window shown again later is + # not repeated, it points at the later turn. + assert last.count("line 250 ") == 1 + # Turns 4, 6 and 8 have their reads paused (three read-only turns in a + # row), so turn 7 holds the last views. + assert "shown again in turn 7" in last + + +def test_a_view_of_a_file_changed_later_is_marked_stale() -> None: + ports = FakePorts( + [ + _reply(1, _a("view", path="src/m.py")), + _reply( + 2, + _a( + "edit", + file_path="src/m.py", + old_string="return 0", + new_string="return a + b", + ), + ), + ], + ) + HandlerDelegatedCodeEditOrchestrator(ports).run(_request(max_turns=3)) + last = ports.prompts[-1] + assert "src/m.py changed in turn 2 after this view" in last + + +def test_one_turns_reads_are_bounded_so_the_newest_turn_is_never_cut() -> None: + """Replays a16a3138 and e3a2c923 again: a turn read 8 to 12 windows of up to + 16 KB, more than the whole history budget, so even the newest turn was shown + cut and the model re-read what it had just read.""" + big = "".join(f"line {i} " + "x" * 40 + "\n" for i in range(1, 1001)) + ports = FakePorts( + [_views(1), _views(2)], files={"src/m.py": "x\n", "src/big.py": big} + ) + HandlerDelegatedCodeEditOrchestrator(ports).run(_request(max_turns=3)) + receipt = next(iter(ports.receipts.values())) + turns = cast("list[dict[str, Any]]", receipt["turns"]) + actions = cast("list[dict[str, Any]]", turns[0]["actions"]) + outputs = [a["output"] for a in actions] + oks = [a["ok"] for a in actions] + assert oks[:2] == [True, True] + assert sum(len(o) for o, ok in zip(outputs, oks, strict=True) if ok) <= 30_000 + assert not oks[-1] + assert "this turn's reads" in outputs[-1] + # The newest turn, and the one before it, are shown whole. + last = ports.prompts[-1] + assert "more characters cut" not in last + assert "line 250 " in last + + +def test_a_view_shrinks_to_what_the_turn_can_still_read() -> None: + from omnimarket.nodes.node_delegated_code_edit_orchestrator.handlers.handler_delegated_code_edit_orchestrator import ( + view_window, + ) + + text = "".join(f"row {i}\n" for i in range(1, 501)) + shown = view_window(text, "a.py", 1, limit=500) + assert len(shown) <= 500 + 200 + assert "[more: view a.py with offset=" in shown + + +def test_reads_pause_after_three_turns_that_read_and_change_nothing() -> None: + """Replays a16a3138 and e3a2c923 a third time: with the history fixed and one + turn's reads bounded, the model still read for 30 turns without editing.""" + view = _a("view", path="src/m.py") + edit = _a( + "edit", file_path="src/m.py", old_string="return 0", new_string="return a + b" + ) + ports = FakePorts( + [ + _reply(1, view), + _reply(2, view), + _reply(3, view), + _reply(4, view, edit), + _reply(5, view), + ], + ) + HandlerDelegatedCodeEditOrchestrator(ports).run(_request(max_turns=5)) + assert "reads are refused this turn" not in ports.prompts[2] + assert "reads are refused this turn" in ports.prompts[3] + receipt = next(iter(ports.receipts.values())) + turns = cast("list[dict[str, Any]]", receipt["turns"]) + fourth = cast("list[dict[str, Any]]", turns[3]["actions"]) + assert [a["ok"] for a in fourth] == [False, True] + assert "change no file" in fourth[0]["output"] + # The edit ended the streak: the next turn reads again. + fifth = cast("list[dict[str, Any]]", turns[4]["actions"]) + assert fifth[0]["ok"] + assert ports.files["src/m.py"] == FIX + + +def test_a_paused_turn_that_still_changes_nothing_allows_one_more_read_turn() -> None: + view = _a("view", path="src/m.py") + ports = FakePorts([_reply(n, view) for n in range(1, 7)]) + HandlerDelegatedCodeEditOrchestrator(ports).run(_request(max_turns=6)) + receipt = next(iter(ports.receipts.values())) + turns = cast("list[dict[str, Any]]", receipt["turns"]) + oks = [cast("list[dict[str, Any]]", t["actions"])[0]["ok"] for t in turns] + assert oks == [True, True, True, False, True, False] + + +def test_an_edit_whose_first_line_alone_carries_an_extra_blank_applies() -> None: + """Replay ea3ee264, re-run 2: copying from ``| def f(`` the model kept the + separator's blank on the first line only; every other line was exact.""" + body = "x = 1\n\n\ndef add(a, b):\n return 0\n" + ports = FakePorts( + [ + _reply( + 1, + _a( + "edit", + file_path="src/m.py", + old_string=" def add(a, b):\n return 0", + new_string=" def add(a, b, c=0):\n return a + b + c", + ), + ) + ], + files={"src/m.py": body}, + ) + HandlerDelegatedCodeEditOrchestrator(ports).run(_request(max_turns=1)) + assert ports.files["src/m.py"] == ( + "x = 1\n\n\ndef add(a, b, c=0):\n return a + b + c\n" + ) + assert [c["status"] for c in _calls(ports)] == ["ok"] + + +def test_replace_in_files_caps_the_files_holding_old_string_not_the_glob() -> None: + """Replay 8e1b5f72 (OMN-20392 AC2, loop 7ea45ffe): the glob + ``src/omnimarket/nodes/*/contract.yaml`` matches more node contracts than + the cap, though fewer hold old_string, and was refused outright; the model + fell back to a /tmp helper script and 117 single edits.""" + files = {f"src/m{n}.py": "other" for n in range(MAX_BULK_FILES + 50)} + files.update({"src/m1.py": "old", "src/m7.py": "old\nold"}) + action = _a("replace_in_files", glob="src/*.py", old_string="old", new_string="new") + ports = FakePorts([_reply(1, action)], files=files, check_passes=[True]) + HandlerDelegatedCodeEditOrchestrator(ports).run(_request(max_turns=1)) + assert sorted(ports.writes) == ["src/m1.py", "src/m7.py"] + assert ports.files["src/m7.py"] == "new\nnew" + call = _calls(ports)[0] + assert call["status"] == "ok" + assert f"2 edited, 0 failed of {MAX_BULK_FILES + 50}" in call["output"] + assert f"{MAX_BULK_FILES + 48} without old_string" in call["output"] From c02ed335bdabfd9a460296f37d85690a945b1201 Mon Sep 17 00:00:00 2001 From: jonahgabriel Date: Sat, 3 Oct 2026 01:10:13 -0400 Subject: [PATCH 2/2] chore(OMN-20291): ticket contract binding each acceptance criterion to the pull request's tests Onex-Lane: code-edit-node-compare Onex-Session: 4380aed584814eab8ef70338fafb073d --- contracts/OMN-20291.yaml | 66 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 contracts/OMN-20291.yaml diff --git a/contracts/OMN-20291.yaml b/contracts/OMN-20291.yaml new file mode 100644 index 0000000000..adac22b89d --- /dev/null +++ b/contracts/OMN-20291.yaml @@ -0,0 +1,66 @@ +--- +schema_version: "1.0.0" +ticket_id: "OMN-20291" +title: "Fix the delegated code edit node's failure classes found by the crush replay comparison" +summary: "The OMN-20291 replay of 26 crush tasks left the delegated code edit node failing 8. Four causes were in the node's loop: a history that forgot what the model had read, so it re-read to the turn cap; absolute paths inside the worktree refused as leaving it; edits whose anchor carried the view separator's blank; and a replace_in_files cap that counted every file a glob matched. Each is fixed and bound to the pull request's own tests." +is_seam_ticket: false +interface_change: false +interfaces_touched: [] +requirements: + - id: "req-code-edit-replay-failure-classes" + statement: |- + The delegated code edit node must not lose the turns a model has read, must use an absolute path inside its own worktree, must apply an edit whose only difference is the view separator's indentation, and must cap a bulk edit by the files it would change. + acceptance: + - id: "AC1" + statement: |- + AC1: every earlier turn stays in the prompt with its action lines, a re-viewed window is not repeated, a view of a file changed later is marked stale, one turn's reads are bounded so the newest turn is never cut, and reads pause after three turns that read and change no file -- falsifier: uv run pytest tests/nodes/node_delegated_code_edit_orchestrator/test_handler_delegated_code_edit_orchestrator.py -q -k "earlier_turn or stale or reads_are_bounded or shrinks or reads_pause or paused_turn or cut_not_dropped or resume" + - id: "AC2" + statement: |- + AC2: an absolute path inside the worktree is applied and recorded as its worktree-relative form, and any other absolute path is still refused -- falsifier: uv run pytest tests/nodes/node_delegated_code_edit_orchestrator/test_handler_delegated_code_edit_orchestrator.py -q -k "absolute_path_inside or normalise_path_maps or path_rules" + - id: "AC3" + statement: |- + AC3: an edit whose old_string matches exactly one place up to a uniform indentation shift, or a shift of its first line alone, is applied with new_string moved back; an ambiguous match still fails; an edit already applied is reported applied and not applied twice -- falsifier: uv run pytest tests/nodes/node_delegated_code_edit_orchestrator/test_handler_delegated_code_edit_orchestrator.py -q -k "indentation or first_line_alone or already_applied or inserted_twice" + - id: "AC4" + statement: |- + AC4: replace_in_files caps a glob by the files that hold old_string, not every file the glob matches -- falsifier: uv run pytest tests/nodes/node_delegated_code_edit_orchestrator/test_handler_delegated_code_edit_orchestrator.py -q -k "replace_in_files" +evidence_requirements: + - kind: "tests" + description: "The delegated code edit node's handler tests pass." + command: "uv run pytest tests/nodes/node_delegated_code_edit_orchestrator/test_handler_delegated_code_edit_orchestrator.py -q" +emergency_bypass: + enabled: false + justification: "" + follow_up_ticket_id: "" +dod_evidence: + - id: "dod-code-edit-replay-ac1" + description: "The history, read-bound and read-pause tests prove the model keeps what it read and is stopped from reading to the turn cap." + source: "manual" + checks: + - check_type: "test_passes" + check_value: "uv run pytest tests/nodes/node_delegated_code_edit_orchestrator/test_handler_delegated_code_edit_orchestrator.py -q -k \"earlier_turn or stale or reads_are_bounded or shrinks or reads_pause or paused_turn or cut_not_dropped or resume\"" + cwd: "${OMNI_HOME}/omnimarket" + binds_ac: ["AC1"] + - id: "dod-code-edit-replay-ac2" + description: "The path tests prove an absolute in-worktree path is used as its relative form and any other is refused." + source: "manual" + checks: + - check_type: "test_passes" + check_value: "uv run pytest tests/nodes/node_delegated_code_edit_orchestrator/test_handler_delegated_code_edit_orchestrator.py -q -k \"absolute_path_inside or normalise_path_maps or path_rules\"" + cwd: "${OMNI_HOME}/omnimarket" + binds_ac: ["AC2"] + - id: "dod-code-edit-replay-ac3" + description: "The anchor tests prove indentation-shifted and already-applied edits are handled and ambiguous ones still fail." + source: "manual" + checks: + - check_type: "test_passes" + check_value: "uv run pytest tests/nodes/node_delegated_code_edit_orchestrator/test_handler_delegated_code_edit_orchestrator.py -q -k \"indentation or first_line_alone or already_applied or inserted_twice\"" + cwd: "${OMNI_HOME}/omnimarket" + binds_ac: ["AC3"] + - id: "dod-code-edit-replay-ac4" + description: "The replace_in_files tests prove the cap counts the files a glob would edit." + source: "manual" + checks: + - check_type: "test_passes" + check_value: "uv run pytest tests/nodes/node_delegated_code_edit_orchestrator/test_handler_delegated_code_edit_orchestrator.py -q -k \"replace_in_files\"" + cwd: "${OMNI_HOME}/omnimarket" + binds_ac: ["AC4"]