Repository navigation
Conversation
|
run-ci |
isaaclab-bot
Bot
added
ci:run-docker
 [High impact] Adds AI model calls and GitHub token generation to backport automation. Fix empty-file resolution before merging; valid backports can currently be rejected even when the model returns the right result. Findings
SummaryThe PR gives the backport resolver more context, release-tree tools, smaller edits, correction rounds, and a way to stop with a reason.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Cherry-pick source change] --> B{Conflict?}
B -->|No| C[Check exact patch and push release]
B -->|Yes| D[Collect source change and conflicts]
D --> E[Model reads release files]
E --> F{Reply}
F -->|Invalid| G[Request correction or try fallback]
G --> E
F -->|Blocked| H[Comment on source PR]
F -->|Accepted| I[Apply edits and write report]
I --> J[Run hooks and check candidate]
J -->|Failure| H
J -->|Pass| K[Continue cherry-pick and open draft]
Reviews (1) · Last reviewed commit: "Accept declared extra edits to cleanly m..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "source_parent": source_parent, | ||
| "source_commit": source, | ||
| "release_base": target, | ||
| "source_description": _git("show", "--no-patch", "--format=%B", source), |
There was a problem hiding this comment.
source_description contains the commit message, not the PR description. The workflow supports merge commits and rebased PRs, whose messages can omit the PR's explanation. The prompt nevertheless tells the model it received that description, leaving it without context needed to choose the right release-side change.
Pass the actual PR body from the event or GitHub API. Keep the commit message as a separate field.
| if name == "read_file": | ||
| output = _git_bytes("show", f"{target}:{path}").decode("utf-8", errors="replace") |
There was a problem hiding this comment.
extra_edits search the current worktree, but read_file only shows the release version from before the cherry-pick. For a cleanly merged path, the model must reconstruct the current file to supply an exact search match. If the relevant source diff was truncated, it cannot inspect those applied changes at all.
Add a read-only way to inspect the current content used by extra_edits, while retaining access to the release version.
| if not isinstance(content, str) or not content: | ||
| raise ConflictResolutionError(f"model returned no text content for {path!r}") |
There was a problem hiding this comment.
_checked_content now rejects an empty string for both write and the result of edit. A source change can clear a file that release also modified, producing a conflict whose correct result is an empty tracked file. Neither action can express that result, and delete is rejected because the source still contains the file. Both models will fail even when they return the right content.
Allow empty strings while retaining the type, size, and NUL checks.
| if not isinstance(content, str) or not content: | |
| raise ConflictResolutionError(f"model returned no text content for {path!r}") | |
| if not isinstance(content, str): | |
| raise ConflictResolutionError(f"model returned non-text content for {path!r}") |
| arguments = json.loads(function.get("arguments") or "{}") | ||
| except json.JSONDecodeError: | ||
| return "error: tool arguments are not valid JSON" | ||
| path = str(arguments.get("path") or "").strip("/") |
There was a problem hiding this comment.
json.loads can return a list, string, or number, but _run_tool immediately calls .get on the result. Model-generated arguments such as [] raise AttributeError instead of returning an error the model can correct. This also bypasses the fallback model and detailed failure report.
Check that the decoded arguments are an object before reading fields. Return a tool error for other shapes.
| arguments = json.loads(function.get("arguments") or "{}") | |
| except json.JSONDecodeError: | |
| return "error: tool arguments are not valid JSON" | |
| path = str(arguments.get("path") or "").strip("/") | |
| arguments = json.loads(function.get("arguments") or "{}") | |
| except json.JSONDecodeError: | |
| return "error: tool arguments are not valid JSON" | |
| if not isinstance(arguments, dict): | |
| return "error: tool arguments must be a JSON object" | |
| path = str(arguments.get("path") or "").strip("/") |
| python3 ../automation/.github/scripts/backport.py validate-candidate \ | ||
| --source_parent "$SOURCE_PARENT" \ | ||
| --source "$SOURCE_SHA" \ | ||
| --target "$TARGET_SHA" | ||
| --target "$TARGET_SHA" \ | ||
| --resolution_report "$RUNNER_TEMP/backport-resolution.json" |
There was a problem hiding this comment.
The new report accepts a resolution that keeps every path unchanged, but the next step always runs git cherry-pick --continue. When release already has the intended behavior and the model correctly chooses keep_target for every conflict, there are no changes to commit. Git rejects the empty cherry-pick, and the workflow incorrectly asks for a manual backport.
Detect this no-change result, end the cherry-pick, and report that no backport change was needed.
| missing_paths = source_paths - candidate_paths | ||
| changed_kept_paths = {path for path in kept_paths if _tree_entry(target, path) != _candidate_entry(candidate, path)} | ||
| if changed_kept_paths: | ||
| raise BackportValidationError( | ||
| "the candidate changes paths declared as kept at the release version: " + _format_paths(changed_kept_paths) | ||
| ) | ||
| declared_paths = kept_paths | extra_paths | ||
| nonmatching_missing_paths = { | ||
| path for path in missing_paths if _tree_entry(source, path) != _candidate_entry(candidate, path) | ||
| path | ||
| for path in missing_paths - declared_paths | ||
| if _tree_entry(source, path) != _candidate_entry(candidate, path) |
There was a problem hiding this comment.
The new kept-path and extra-path exceptions have no automated tests. The reported offline checks cover successful historical resolutions, but not the checks that must still reject a bad candidate. Future changes could weaken these checks or reject valid backports unnoticed.
Add tests using temporary Git repositories to prove that:
- Kept paths preserve both content and mode.
- Undeclared extra paths fail.
- A declared extra path can restore a cleanly merged file.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
Needs attention: Design and architecture (1), Implementation (2), Style consistency (1). Other reviewed categories look good.
- PR description: The body mislabels the merge commit message as the PR description and a 300,000-character prefix as the full diff; correct both claims and add exact baseline/candidate commands and revisions for latency/cost figures.
- Design and architecture: Release-tree tools cannot inspect the worktree content used for extra edits, so generated searches can target stale text; expose the actual edit basis read-only.
- Scope and over-engineering: No over-engineering issues.
- API: API changes look good.
- Compatibility and deprecation: No breaking changes identified.
- Implementation: Model-selected extra paths authorize their own validation exemption, allowing arbitrary existing repository files to be committed; enforce an independently derived allowlist before applying or validating extra edits.
- Style consistency: Repeated numeric caps can drift from tool descriptions and summary behavior; define named module constants and interpolate them at each use.
- Test quality: Test quality looks good.
Significant concerns. Posted 4 inline findings.
Automated review; human maintainers own approval decisions.
| "source_parent": source_parent, | ||
| "source_commit": source, | ||
| "release_base": target, | ||
| "source_description": _git("show", "--no-patch", "--format=%B", source), |
There was a problem hiding this comment.
🟡 Warning · Implementation — Commit message labeled PR description
%B of the merged commit is not the PR description for rebased merges or default squash messages, yet the prompt presents it as such. Pass github.event.pull_request.body instead.
| if not isinstance(edit, dict) or set(edit) != {"path", "edits", "reason"}: | ||
| raise ConflictResolutionError("each extra edit must contain exactly path, edits, and reason") | ||
| path = edit["path"] | ||
| if not isinstance(path, str) or path in files or path in {item["path"] for item in validated_extra}: |
There was a problem hiding this comment.
🟡 Warning · Implementation — Extra edits accept any repository path
Extra paths are only checked for existence and are exempted from validate-candidate's outside-the-PR check, so model-chosen edits to any file, including .github/ workflows, get committed. Restrict the allowed extra paths.
| if path: | ||
| _validate_repository_path(path) | ||
| if name == "read_file": | ||
| output = _git_bytes("show", f"{target}:{path}").decode("utf-8", errors="replace") |
There was a problem hiding this comment.
🟡 Warning · Design Architecture — read_file basis differs from extra edits
read_file returns the release blob, but extra edits apply to worktree content, which already carries the cleanly merged source change. Search text taken from this tool can fail the exactly-once check.
| output = _git_bytes("show", f"{target}:{path}").decode("utf-8", errors="replace") | ||
| elif name == "list_dir": | ||
| entries = _git("ls-tree", "-r", "--name-only", target, "--", path).splitlines() | ||
| output = "\n".join(entries[:200]) + ("\n..." if len(entries) > 200 else "") |
There was a problem hiding this comment.
🟡 Warning · Style Consistency — Hard-coded caps bypass module constants
list_dir's 200 and grep's 100 duplicate the numbers stated in their tool descriptions, and write_summary repeats 50 three times. Define _MAX_* constants beside the adjacent limits so the values cannot drift.
Summary
The backport bot's resolver sent the model only the conflicted files and forced a full rewrite of each. The model could not see the PR's intent or release's layout, so it imported develop-only code: #8509 recreated
install_ci/conftest.pyfixtures andtest_workspace.pyfrom #8128, which release never received.azure/openai/gpt-6.1-sol, falling back toazure/anthropic/claude-opus-5-5. TheNVIDIA_BACKPORT_MODEL/NVIDIA_BACKPORT_FALLBACK_MODELrepository variables still override.edit,write,keep_targetordeleteper conflicted path.extra_editsfor release's equivalent of code that exists only on develop.blockedto fail with a reason instead of opening a wrong draft.read_file,list_dir,grep), capped at 10 calls. Up to 2 correction rounds when validation rejects a reply. Reasoning effort is medium.backport.py validate-candidate --resolution_reportaccepts declared kept paths, which must equal release, and declared extra paths. All other checks are unchanged.blocked, the bot comments on the source PR with the reason and the workflow run link, so a manual backport is not missed. The app token is created earlier so this step can use it.Validation
Ran this PR's resolver against the inference API on past conflicted backports, using
azure/openai/gpt-6.1-solat medium effort:test_workspace.pyconftest.py, lefttest_workspace.pyabsent, and moved the ARM re-enable into the release uv_run test. It also restored the cleanly mergedcartpole_training_smoke.pydocstring, which a human had fixed by hand; its version is one docstring line off from the human fix.blockedreply against a real model.