Make backports apply the complete source change safely - #76
Make backports apply the complete source change safely#76sarthakaggarwal97 wants to merge 7 commits into
Conversation
GitHub's merge_commit_sha has different meanings by merge method: a real merge commit for merge, the aggregate commit for squash, and only the final rewritten commit for a multi-commit rebase-and-merge. Cherry-picking it blindly backports a fraction of a rebase-merged pull request while reporting success. plan_source_change proves what the merge SHA is before anything picks it. A merge commit is identified by its parents; a squash is confirmed by exact patch identity against the aggregated source commits. A multi-commit pull request whose merge SHA does not match its source aggregate was rebase-merged, and planning fails closed with an error directing it to a manual backport — replaying rebased series is intentionally unsupported, and every plan carries exactly one authoritative commit by construction. When the API pages out a large PR's commit listing, classification falls back to the fetched PR head tip, which is all the patch comparison needs. Empty or incomplete histories, duplicate SHAs, missing commits after fetch, and ambiguous merge bases all fail closed, and the guards are mutation-tested. Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
The manual backport and the sweep each had their own apply engine (cherry_pick.py and sweep_apply.py), and they had drifted: missing-test adaptation, deterministic empty-skip reasons, and the conflict-file cap existed only in the sweep; only the manual path could handle a pull request without a merge SHA. apply_candidate in application.py now serves both entry points from the source-change plan, so every future fix lands once. The unified engine also hardens failure handling. The starting HEAD is captured unconditionally and any unexpected failure aborts and resets, so a failed candidate can never strand a dirty tree that poisons later candidates in the same sweep. A cherry-pick --abort that itself fails (possible when the pick died before creating sequencer state, e.g. an untracked file collision with validation build artifacts) falls back to reset --hard HEAD — and when even that fails, the candidate is an error, the result records the unrestored worktree, and the sweep aborts the branch rather than applying later candidates to a poisoned tree. A hard cherry-pick failure is an error, never misreported as already-applied. Binary conflicts skip the candidate up front with the paths named instead of failing confusingly after text resolution. Partial-application state lives in one _ApplyState owned by apply_candidate. AI disclosure is now accurate in both directions. Involvement keys on actual LLM resolutions (resolution source, not mere presence) and on missing-test adaptation, which sets resolved_by_ai even without conflict resolutions; automatic resolutions and dropped test files no longer claim AI authorship. create_backport_pr honors the flag for the ai-resolved-conflicts label and human-review disclaimer, an AI Adaptation section describes what was adapted, and body rebuilds preserve the disclosure. The skipped-existing message distinguishes 'already applied' from 'does not apply to this branch'. Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe backport system now plans source histories, applies candidates through a centralized pipeline, adapts missing tests in a sandbox, propagates AI and validation metadata, supports configurable test patterns, improves rollback handling, and scopes workflow tokens dynamically per repository owner. ChangesBackport pipeline
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
scripts/backport/source_change.py (2)
63-96: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant PR-head fetch on the incomplete-pagination path.
_fetch_pr_head_tipalready fetches+refs/pull/N/head:refs/valkey-ci-agent/backport/N/head, so whensource_commits_completeis false the block at Lines 76-88 refetches the same refspec. Skipping it when the tip was just fetched saves a network round trip on exactly the large-PR case this path targets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/source_change.py` around lines 63 - 96, The missing-commit recovery in the source-change flow redundantly fetches the PR head after _fetch_pr_head_tip already fetched it for incomplete pagination. Track whether that fetch occurred, and skip the _git fetch block when source_commits_complete is false while retaining the existing missing-commit validation and error for other cases.
250-255: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueResolve revisions before comparing
_commit_parentsoutput.
rev-list --parents -n 1outputs the resolved commit object ID, so abbreviated SHAs/tags/branches can raisecould not resolve commiteven when_commit_existsaccepts them. Sinceplan_source_changereceives unvalidated commit SHAs, make_commit_parentsnormalize the input first or compare a normalized resolved ID.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/source_change.py` around lines 250 - 255, Update _commit_parents to resolve the supplied revision to its canonical commit ID before validating rev-list output, using the repository’s existing revision-resolution mechanism. Compare fields[0] against that normalized ID while preserving the existing parent tuple and error behavior, so abbreviated SHAs, tags, and branches accepted by _commit_exists work correctly.scripts/backport/application.py (1)
164-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
_application_resulthere for consistency.Every other failure return in this module goes through
_application_result; this one constructsCandidateResultpositionally, so a future field default change has to be remembered twice.♻️ Proposed tweak
except (SourceChangeError, subprocess.CalledProcessError) as exc: - return CandidateResult(candidate.source_pr_number, candidate.source_pr_title, "error", str(exc)) + return _application_result(candidate, "error", str(exc))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/application.py` around lines 164 - 174, Update the exception handler around source-plan preparation to return through the existing _application_result helper instead of constructing CandidateResult positionally, preserving the current error status and exception message while centralizing result creation.scripts/backport/git.py (2)
80-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
read_index_stageswallows failures as empty content.A missing stage, a
git showerror, and a genuinely empty blob all return"".scripts/backport/application.pycompensates by callingindex_stage_existsseparately (Line 328), but any future caller reading only the content cannot tell "absent" from "empty". A sentinel (str | None) or a paired(ok, content)return would make the contract explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/git.py` around lines 80 - 94, Update read_index_stage to distinguish a missing or failed Git index stage from a genuinely empty blob by returning an explicit sentinel such as str | None (or an equivalent success/content pair). Preserve successful non-empty and empty stdout values, and update existing callers such as index-stage handling in application.py to use the new contract instead of relying on "" for failure.
15-40: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo bounded timeout on any Git subprocess helper in the new modules. Both helper layers shell out to Git without
timeout=, and both are used for network operations (fetch,push), so a stalled remote hangs the backport job until the runner-level timeout with no actionable diagnostic.
scripts/backport/git.py#L15-L40: add atimeoutparameter (with a sane default) torun_gitand surfacesubprocess.TimeoutExpiredas a clear failure.scripts/backport/source_change.py#L295-L331: pass the same boundedtimeoutin_gitand_git_bytes, convertingsubprocess.TimeoutExpiredintoSourceChangeErrorsoapply_candidatereports it as a candidate error rather than an unexpected crash.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/git.py` around lines 15 - 40, Bound all Git subprocesses with a shared sane timeout: update scripts/backport/git.py lines 15-40 in run_git to accept and pass a timeout, and convert subprocess.TimeoutExpired into a clear failure; update scripts/backport/source_change.py lines 295-331 in _git and _git_bytes to use the same bounded timeout and translate timeout exceptions into SourceChangeError so apply_candidate reports a candidate error.scripts/backport/models.py (1)
76-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid mixing
frozen=Truewith mutablelistfields.
commit_shasremains mutable through the frozen dataclass, and hashing any candidate with the generated__hash__raisesTypeError: unhashable type: 'list'. Use atuple[str, ...]field or removefrozen=Trueunless immutable candidates/hashing are required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/models.py` around lines 76 - 88, Update the BackportCandidate dataclass so its frozen instances contain only immutable fields: change commit_shas from a mutable list to a tuple[str, ...] and use an appropriate immutable default factory/value. Preserve callers’ expected commit SHA collection behavior while ensuring generated hashing cannot fail.scripts/backport/main.py (1)
333-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnexpected-outcome path skips the source-PR notification.
Every other terminal branch here (
skipped-existing,error,skipped-conflict) posts back to the source PR, so a futureCandidateOutcome(e.g.skipped-validation-failedleaking out ofapply_candidate) would fail silently from the requester's perspective.♻️ Post the same failure comment as the other error path
if application_result.outcome != "applied": msg = ( "Candidate application returned unexpected outcome: " f"{application_result.outcome}" ) logger.error(msg) + _post_comment(repo, source_pr_number, f"Backport failed: {msg}") return BackportResult( outcome="error", error_message=msg, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/main.py` around lines 333 - 342, Update the unexpected-outcome branch in the backport flow to post the same failure notification to the source PR as the other terminal error paths before returning BackportResult. Reuse the existing notification mechanism and preserve the current error outcome and message.tests/test_backport_pr_creator.py (1)
841-874: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLine 866 asserts on a phrase no code path emits.
The disclaimer text is "AI was used to resolve conflicts or adapt this backport.";
"conflicts in this backport were resolved"appears nowhere inbuild_pr_body, so that assertion passes unconditionally and guards nothing. Assert on the real string, or drop the line. Also, the docstring mentions the label decision, but this test only exercises the body — the label path iscreate_backport_pr.♻️ Make the negative assertion meaningful
assert "### AI Adaptation" in body assert "tests/unit/networking.tcl" in body assert "Human Review Required" in body - assert "conflicts in this backport were resolved" not in body + assert "AI was used to resolve conflicts or adapt this backport" in body🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_backport_pr_creator.py` around lines 841 - 874, Update test_ai_adaptation_without_resolutions_gets_summary_and_disclaimer to assert that the actual disclaimer text emitted by build_pr_body, “AI was used to resolve conflicts or adapt this backport.”, is absent when ai_involved is false; remove the ineffective assertion on the nonexistent phrase. Keep the body-summary and human-review assertions unchanged, and do not claim label behavior unless testing create_backport_pr.tests/test_backport_source_change.py (1)
37-50: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRepo bootstrap here omits
commit.gpgsign=false.
tests/test_backport_application.pydisables signing in_init_repo; this module does not, so every_commitfails on a host or runner withcommit.gpgsign=trueset globally. The same four-line bootstrap is also repeated intest_squash_ignores_target_updates_merged_into_source,test_rebase_with_whitespace_only_commit_is_refused,test_disconnected_multi_commit_history_is_refused,test_ambiguous_merge_base_explains_classification_failure,test_incomplete_commit_page_still_classifies_squash, andtest_source_commit_absent_from_pr_head_fails_after_fetch— extracting a helper fixes all of them at once.♻️ Shared init helper
+def _init_repo(repo: Path) -> None: + repo.mkdir() + _git(repo, "init", "-q", "-b", "main") + _git(repo, "config", "user.name", "Test") + _git(repo, "config", "user.email", "test@example.com") + _git(repo, "config", "commit.gpgsign", "false") + + `@pytest.fixture` def history(tmp_path: Path) -> tuple[Path, str, str, str]: repo = tmp_path / "repo" - repo.mkdir() - _git(repo, "init", "-q", "-b", "main") - _git(repo, "config", "user.name", "Test") - _git(repo, "config", "user.email", "test@example.com") + _init_repo(repo) _commit(repo, "base.txt", "base\n", "base")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_backport_source_change.py` around lines 37 - 50, Update the repository bootstrap used by the history fixture and the repeated setup in the listed backport-source tests to configure commit.gpgsign=false before creating commits. Extract the shared initialization into a helper and reuse it across those tests, preserving their existing repository, branch, and commit histories.scripts/backport/sweep.py (1)
162-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
commits_pagefor the node list.The connection is now fetched twice, and the older access (
content.get("commits", {})) still breaks if the field comes back as an explicitnull, unlike the newor {}form.♻️ Single, null-safe access
- commits = [ - node.get("commit", {}).get("oid", "") - for node in (content.get("commits", {}).get("nodes") or []) - ] commits_page = content.get("commits") or {} + commits = [ + (node.get("commit") or {}).get("oid", "") + for node in (commits_page.get("nodes") or []) + ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/sweep.py` around lines 162 - 166, Update the commit extraction in the backport flow to derive its node list from the already initialized null-safe commits_page value. Remove the second content.get("commits", ...) access and preserve the existing commit OID extraction behavior.tests/test_backport_main.py (1)
521-636: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo coverage for the new
skipped-conflictbranch.
run_backportgained aconflicts-unresolvedpath (scripts/backport/main.pylines 299-332) that derivesfiles_conflicted/files_resolved/files_unresolvedby intersectingconflicting_filespaths with resolvedresolutions, then posts a comment and job summary. That counting logic is the kind that silently drifts; a case with one resolved and one unresolved conflicted path would pin it down.Want me to draft that test?
Also applies to: 925-941
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_backport_main.py` around lines 521 - 636, Add a run_backport test covering the conflicts-unresolved/skipped-conflict path with two conflicting files, one present in resolutions and one unresolved. Assert the derived files_resolved, files_unresolved, and files_conflicted reporting in the posted comment and job summary, including the expected result outcome. Reuse the existing backport test fixtures and mocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/backport/missing_test_adaptation.py`:
- Around line 111-141: Replace the full-worktree byte snapshots around
run_agent_func and the corresponding later snapshot flow with digest-only
snapshots for change detection, so regular-file contents are not buffered twice.
Preserve a content-carrying snapshot only for the small import_snapshots set
needed by restore_paths, and have snapshot walking skip git-ignored build
outputs where supported; keep changed_paths behavior and downstream rereading of
changed files unchanged.
- Around line 99-101: Update the existing_test_paths initialization in the
missing-test adaptation flow to request the complete list from
list_existing_test_paths by disabling its default limit. Keep the capped listing
behavior for prompt generation, but ensure invalid_sandbox_test_paths validates
changed paths against all existing test files.
In `@scripts/backport/source_change.py`:
- Around line 212-231: Update the terminal error handling in the source-change
planning flow after the source and merge patch-ID comparison so patch-ID
mismatches use a diagnostic that also covers an advanced base branch, rather
than attributing the failure solely to a rebase merge. Preserve the existing
fail-closed behavior and successful squash plan returned by the matching branch.
In `@tests/test_backport_sweep.py`:
- Around line 182-262: Add an assertion to
test_apply_candidate_does_not_invoke_resolver_for_mixed_binary_conflict
verifying that the recorded subprocess calls include ["git", "cherry-pick",
"--abort"], matching the rollback check in the sibling binary-conflict test.
---
Nitpick comments:
In `@scripts/backport/application.py`:
- Around line 164-174: Update the exception handler around source-plan
preparation to return through the existing _application_result helper instead of
constructing CandidateResult positionally, preserving the current error status
and exception message while centralizing result creation.
In `@scripts/backport/git.py`:
- Around line 80-94: Update read_index_stage to distinguish a missing or failed
Git index stage from a genuinely empty blob by returning an explicit sentinel
such as str | None (or an equivalent success/content pair). Preserve successful
non-empty and empty stdout values, and update existing callers such as
index-stage handling in application.py to use the new contract instead of
relying on "" for failure.
- Around line 15-40: Bound all Git subprocesses with a shared sane timeout:
update scripts/backport/git.py lines 15-40 in run_git to accept and pass a
timeout, and convert subprocess.TimeoutExpired into a clear failure; update
scripts/backport/source_change.py lines 295-331 in _git and _git_bytes to use
the same bounded timeout and translate timeout exceptions into SourceChangeError
so apply_candidate reports a candidate error.
In `@scripts/backport/main.py`:
- Around line 333-342: Update the unexpected-outcome branch in the backport flow
to post the same failure notification to the source PR as the other terminal
error paths before returning BackportResult. Reuse the existing notification
mechanism and preserve the current error outcome and message.
In `@scripts/backport/models.py`:
- Around line 76-88: Update the BackportCandidate dataclass so its frozen
instances contain only immutable fields: change commit_shas from a mutable list
to a tuple[str, ...] and use an appropriate immutable default factory/value.
Preserve callers’ expected commit SHA collection behavior while ensuring
generated hashing cannot fail.
In `@scripts/backport/source_change.py`:
- Around line 63-96: The missing-commit recovery in the source-change flow
redundantly fetches the PR head after _fetch_pr_head_tip already fetched it for
incomplete pagination. Track whether that fetch occurred, and skip the _git
fetch block when source_commits_complete is false while retaining the existing
missing-commit validation and error for other cases.
- Around line 250-255: Update _commit_parents to resolve the supplied revision
to its canonical commit ID before validating rev-list output, using the
repository’s existing revision-resolution mechanism. Compare fields[0] against
that normalized ID while preserving the existing parent tuple and error
behavior, so abbreviated SHAs, tags, and branches accepted by _commit_exists
work correctly.
In `@scripts/backport/sweep.py`:
- Around line 162-166: Update the commit extraction in the backport flow to
derive its node list from the already initialized null-safe commits_page value.
Remove the second content.get("commits", ...) access and preserve the existing
commit OID extraction behavior.
In `@tests/test_backport_main.py`:
- Around line 521-636: Add a run_backport test covering the
conflicts-unresolved/skipped-conflict path with two conflicting files, one
present in resolutions and one unresolved. Assert the derived files_resolved,
files_unresolved, and files_conflicted reporting in the posted comment and job
summary, including the expected result outcome. Reuse the existing backport test
fixtures and mocks.
In `@tests/test_backport_pr_creator.py`:
- Around line 841-874: Update
test_ai_adaptation_without_resolutions_gets_summary_and_disclaimer to assert
that the actual disclaimer text emitted by build_pr_body, “AI was used to
resolve conflicts or adapt this backport.”, is absent when ai_involved is false;
remove the ineffective assertion on the nonexistent phrase. Keep the
body-summary and human-review assertions unchanged, and do not claim label
behavior unless testing create_backport_pr.
In `@tests/test_backport_source_change.py`:
- Around line 37-50: Update the repository bootstrap used by the history fixture
and the repeated setup in the listed backport-source tests to configure
commit.gpgsign=false before creating commits. Extract the shared initialization
into a helper and reuse it across those tests, preserving their existing
repository, branch, and commit histories.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c250a09c-a6af-4cf2-8b91-405be7d0b11c
📒 Files selected for processing (22)
docs/architecture.mdscripts/backport/application.pyscripts/backport/cherry_pick.pyscripts/backport/git.pyscripts/backport/main.pyscripts/backport/missing_test_adaptation.pyscripts/backport/models.pyscripts/backport/pr_creator.pyscripts/backport/revert_commit.pyscripts/backport/source_change.pyscripts/backport/sweep.pyscripts/backport/sweep_apply.pyscripts/backport/sweep_git.pyscripts/backport/sweep_models.pyscripts/backport/sweep_reporting.pyscripts/backport/sweep_validation.pytests/test_backport_application.pytests/test_backport_cherry_pick.pytests/test_backport_main.pytests/test_backport_pr_creator.pytests/test_backport_source_change.pytests/test_backport_sweep.py
💤 Files with no reviewable changes (3)
- tests/test_backport_cherry_pick.py
- scripts/backport/cherry_pick.py
- scripts/backport/sweep_apply.py
- Validate adapted test paths against the full test listing instead of the prompt-capped subset, with a regression test for edits beyond the cap - Detect sandbox changes with chunked digests instead of buffering every file's bytes twice; content is captured only for the restored paths - Widen the patch-id mismatch diagnostic to cover an advanced base branch, not just rebase merges - Assert cherry-pick rollback in the mixed binary conflict test Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_backport_sweep.py`:
- Around line 2130-2141: Update fake_run_agent in the backport sweep test to
inspect _prompt and assert that tests/unit/zzz.tcl is absent from the capped
prompt. Keep the existing sandbox edit and successful result setup unchanged,
while ensuring the test fails if prompt capping regresses and includes that
file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 36a07330-8632-4cb0-8551-46dea606f792
📒 Files selected for processing (3)
scripts/backport/missing_test_adaptation.pyscripts/backport/source_change.pytests/test_backport_sweep.py
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/backport/source_change.py
- scripts/backport/missing_test_adaptation.py
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Protect candidate worktrees and adaptation sandboxes, preserve AI repair provenance, make test layouts and workflow ownership repository-aware, and persist source PR identity across merge styles. Rename the shared application, source planning, and Git command modules for clarity. Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/backport/candidate_apply.py (1)
695-713: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRollback can raise
FileNotFoundErrorwhen a starting untracked file's parent directory no longer exists.
_abort_and_rollbackrestores every pre-existing untracked file via_safe_restore_path(...).write_bytes(content), but_safe_restore_pathnever creates missing parent directories — it only validates for symlink escapes. If a candidate's failed attempt (conflict resolution, missing-test adaptation, etc.) removes the parent directory of a starting untracked file, the restore write raises, propagates out of_abort_and_rollback, and the outer exception handler's second rollback attempt fails the same way — yieldingworktree_restored=False, whichsweep._process_branchtreats as fatal for the whole branch.🐛 Proposed fix
for path, content in starting_untracked_files.items(): destination = _safe_restore_path(Path(repo_dir), path) + destination.parent.mkdir(parents=True, exist_ok=True) destination.write_bytes(content)Also applies to: 731-744
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/candidate_apply.py` around lines 695 - 713, Update _abort_and_rollback so each starting untracked file’s parent directory is recreated before _safe_restore_path(...).write_bytes(content). Preserve _safe_restore_path’s existing safety validation, and ensure restoration succeeds even when the failed operation removed one or more parent directories..github/workflows/backport-poll.yml (1)
108-120: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScope the GitHub App token to the target repo, not the whole owner.
The poll matrix now uses dynamic
matrix.repo_ownerwithoutrepositories:onactions/create-github-app-token, so the generated token is scoped to every repository installed for that owner. That token is also passed through to the Python code that processesmatrix.repo, widening the blast radius. Add repo scoping in the poll/mark-done/Sweep workflows’generate-tokensteps, using the bare repo name that goes withmatrix.repo_owner(e.g.matrix.repo.split('/', 1)[1]).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/backport-poll.yml around lines 108 - 120, Scope the generate-token GitHub App tokens to the target repository by adding the repositories input with the bare repository name derived from matrix.repo. Apply this in .github/workflows/backport-poll.yml lines 108-120 and .github/workflows/backport-mark-done-poll.yml lines 93-104, while retaining matrix.repo_owner as the owner.Source: Linters/SAST tools
🧹 Nitpick comments (2)
scripts/backport/missing_test_adaptation.py (1)
35-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
**glob patterns givenfnmatchsemantics.
is_test_path()matches paths withfnmatch.fnmatchcase, and Python'sfnmatchmodule does not treat/as special — a bare*already matches across directory separators (confirmed in the official docs: "the filename separator ('/' on Unix) is not special to this module"). Sodir/**/*.extmatches exactly the same set of paths asdir/*.ext; the**variants add no coverage and could mislead a future maintainer into assuming shell-glob semantics (where*doesn't cross/) apply here.
scripts/backport/missing_test_adaptation.py#L35-L41: drop the redundant"tests/**/*.tcl"entry fromDEFAULT_TEST_PATH_PATTERNS(or add a short comment noting*already matches nested paths under this module's fnmatch-based matcher).repos.yml#L34-L40: drop the redundant"testing/**/*.cc","vmsdk/testing/**/*.cc", and"integration/**/test_*.py"entries, since the corresponding non-**patterns already match nested paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/missing_test_adaptation.py` around lines 35 - 41, The fnmatch-based path patterns contain redundant ** glob entries. In scripts/backport/missing_test_adaptation.py lines 35-41, remove tests/**/*.tcl from DEFAULT_TEST_PATH_PATTERNS; in repos.yml lines 34-40, remove testing/**/*.cc, vmsdk/testing/**/*.cc, and integration/**/test_*.py, preserving the corresponding non-** patterns.scripts/backport/sweep.py (1)
548-556: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
_head_shahelper — consider centralizing ingit_commands.py.
candidate_apply.pyalready defines a_head_sha(repo_dir, run_process=...)helper used insideapply_candidate's pipeline. This module now adds a second, near-identical implementation (hardcoded tosubprocess.run). Sincegit_commands.pywas introduced precisely to hold shared Git primitives (run_git,has_staged_changes,index_stage_exists,read_index_stage), moving_head_shathere (with an injectablerun_process) would remove the duplication and keep both call sites consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/sweep.py` around lines 548 - 556, Centralize the duplicate _head_sha helper in git_commands.py, matching candidate_apply.py’s injectable run_process interface and behavior. Remove the local implementation from sweep.py, import and reuse the shared helper at its call sites, and preserve existing HEAD SHA retrieval semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/backport.yml:
- Line 97: Restrict both App token generations to the intended repository by
adding the repository restriction to the token configuration in
.github/workflows/backport.yml at lines 97-97 using the parsed repo_full_name,
and in .github/workflows/backport-sweep.yml at lines 111-111 using matrix.repo;
keep the existing owner values unchanged.
In `@scripts/backport/utils.py`:
- Around line 41-55: Update pr_numbers_from_commit_messages so
Backport-Source-PR is parsed only within the structured trailer block at the end
of each message, not from arbitrary body prose; preserve subject parsing and
trailer extraction, and add a negative test covering prose or “## Needs
attention” content so it does not produce a PR number.
---
Outside diff comments:
In @.github/workflows/backport-poll.yml:
- Around line 108-120: Scope the generate-token GitHub App tokens to the target
repository by adding the repositories input with the bare repository name
derived from matrix.repo. Apply this in .github/workflows/backport-poll.yml
lines 108-120 and .github/workflows/backport-mark-done-poll.yml lines 93-104,
while retaining matrix.repo_owner as the owner.
In `@scripts/backport/candidate_apply.py`:
- Around line 695-713: Update _abort_and_rollback so each starting untracked
file’s parent directory is recreated before
_safe_restore_path(...).write_bytes(content). Preserve _safe_restore_path’s
existing safety validation, and ensure restoration succeeds even when the failed
operation removed one or more parent directories.
---
Nitpick comments:
In `@scripts/backport/missing_test_adaptation.py`:
- Around line 35-41: The fnmatch-based path patterns contain redundant ** glob
entries. In scripts/backport/missing_test_adaptation.py lines 35-41, remove
tests/**/*.tcl from DEFAULT_TEST_PATH_PATTERNS; in repos.yml lines 34-40, remove
testing/**/*.cc, vmsdk/testing/**/*.cc, and integration/**/test_*.py, preserving
the corresponding non-** patterns.
In `@scripts/backport/sweep.py`:
- Around line 548-556: Centralize the duplicate _head_sha helper in
git_commands.py, matching candidate_apply.py’s injectable run_process interface
and behavior. Remove the local implementation from sweep.py, import and reuse
the shared helper at its call sites, and preserve existing HEAD SHA retrieval
semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fa902245-86f0-4cbf-b045-115495a2d9df
📒 Files selected for processing (30)
.github/workflows/backport-mark-done-poll.yml.github/workflows/backport-poll.yml.github/workflows/backport-sweep.yml.github/workflows/backport.yml.github/workflows/release-notes-cut.ymldocs/architecture.mdexamples/repos.ymlrepos.ymlscripts/backport/candidate_apply.pyscripts/backport/diff_comments.pyscripts/backport/git_commands.pyscripts/backport/main.pyscripts/backport/mark_done.pyscripts/backport/matrix.pyscripts/backport/missing_test_adaptation.pyscripts/backport/registry.pyscripts/backport/revert_commit.pyscripts/backport/source_plan.pyscripts/backport/sweep.pyscripts/backport/sweep_git.pyscripts/backport/sweep_validation.pyscripts/backport/utils.pytests/test_backport_candidate_apply.pytests/test_backport_mark_done.pytests/test_backport_missing_test_adaptation.pytests/test_backport_source_plan.pytests/test_backport_sweep.pytests/test_diff_comments.pytests/test_matrix.pytests/test_registry.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/architecture.md
Scope App tokens to each target repository, parse source PR metadata only from terminal trailers, and make rollback restoration resilient to removed directories. Remove redundant glob entries and share HEAD resolution without expanding the backport surface. Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/backport/sweep_validation.py (1)
85-92: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new return type.
The function now returns
ValidationOutcome, but the docstring still says "Returns (green, output)". Callers cannot discoverresolutionsandai_summaryfrom this text. State the returned type and the extra fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/sweep_validation.py` around lines 85 - 92, Update the docstring for the validation function returning ValidationOutcome to document the actual return type and describe its resolutions and ai_summary fields, replacing the outdated “(green, output)” wording while preserving the existing repair behavior description.
🧹 Nitpick comments (8)
scripts/backport/pr_creator.py (1)
362-363: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGate the AI Adaptation section on
any_ai_changesfor consistency.Line 365 uses
any_ai_changes, but Line 362 usesai_involved. If a caller passesai_involved=Falsetogether with LLM-resolved results and a non-emptyai_summary, the body shows "Human Review Required" and omits "AI Adaptation". The current callers always pass the combined flag, so behavior matches today. Using one flag prevents that divergence later.♻️ Proposed change
- if ai_summary and ai_involved: + if ai_summary and any_ai_changes: sections.append("### AI Adaptation\n\n" + ai_summary)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/pr_creator.py` around lines 362 - 363, Update the AI Adaptation section condition in the section-building logic to use any_ai_changes instead of ai_involved, matching the existing Human Review Required condition and keeping both sections consistent.tests/test_backport_pr_creator.py (1)
866-866: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert a string that
build_pr_bodycan actually emit.
build_pr_bodynever produces the text "conflicts in this backport were resolved". This negative assertion always passes, so it protects nothing. Assert against a real string instead, for example the "Conflict Details" heading, which must be absent whenresolution_resultsisNone.💚 Proposed change
- assert "conflicts in this backport were resolved" not in body + assert "### Conflict Details" not in body🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_backport_pr_creator.py` at line 866, Update the assertion in the test covering build_pr_body to check for the actual “Conflict Details” heading, asserting it is absent when resolution_results is None. Remove the ineffective assertion against text that build_pr_body never emits.scripts/backport/missing_test_adaptation.py (1)
442-466: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm that a deleted sandbox test file is intentionally fatal.
changed_snapshot_pathsalso reports deletions. For a deleted path, the first condition passes, thensafe_regular_filereturnsNone, so the path is reported as an invalid generated test path and the candidate fails closed. That is safe, but the message says "invalid generated test path(s)", which does not describe a deletion. Consider a distinct summary for removed files so the sweep report explains the real cause.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/missing_test_adaptation.py` around lines 442 - 466, Update invalid_sandbox_test_paths and its reporting flow to distinguish deleted sandbox test files from other invalid generated test paths. Track paths absent from sandbox_before separately, preserve the fail-closed behavior for deletions, and emit a distinct sweep summary explaining that files were removed.tests/test_backport_main.py (1)
316-325: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
apply_candidatemock parameters.These decorators now patch
apply_candidate, but the injected parameter is still namedmock_executor_cls. The name suggests a class-level cherry-pick executor. Rename it tomock_apply_candidate, as the newer test at Line 529 already does.Also applies to: 388-399, 455-464
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_backport_main.py` around lines 316 - 325, Rename the mock parameter corresponding to the patched apply_candidate decorator from mock_executor_cls to mock_apply_candidate in test_clean_cherry_pick_returns_success and the similarly affected tests at the referenced locations, preserving decorator order and all other parameters.scripts/backport/sweep.py (2)
463-476: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead the outcome fields directly.
validate_branch_with_optional_repairreturnsValidationOutcome, sovalidation_outcome.resolutionsandvalidation_outcome.ai_summaryare always present. Thegetattrdefaults suppress type checking and hide future signature changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/sweep.py` around lines 463 - 476, Update the handling of validation_outcome in validate_branch_with_optional_repair’s caller to access ValidationOutcome fields directly: replace the getattr calls for resolutions and ai_summary with direct attribute access, preserving the existing defaults only where they are explicitly required by the surrounding logic.
434-461: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReset to a captured SHA instead of counting commits.
rollback_refderives the reset target fromlen(candidate_result.applied_commits). This assumes the candidate created exactly one commit per applied source commit, an invariant owned bycandidate_apply. If that invariant changes, the sweep resets to the wrong commit and drops previously kept candidates without any error.Capture HEAD before
apply_candidateand reset to that SHA.♻️ Proposed change
+ pre_candidate_head = head_sha(tmpdir) candidate_result = apply_candidate( tmpdir, candidate, @@ - applied_commit_count = max( - 1, - len(candidate_result.applied_commits), - ) - rollback_ref = ( - "HEAD^" - if applied_commit_count == 1 - else f"HEAD~{applied_commit_count}" - ) - _run_git(tmpdir, "reset", "--hard", rollback_ref) + _run_git(tmpdir, "reset", "--hard", pre_candidate_head)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/sweep.py` around lines 434 - 461, Capture the current HEAD SHA immediately before applying each candidate, and use that captured SHA as the rollback target when validation fails. Update the candidate application flow around candidate_apply and replace the applied_commits-based rollback_ref calculation with a hard reset to the pre-application SHA, preserving previously retained candidates regardless of how many commits the candidate creates.scripts/backport/candidate_apply.py (1)
564-573: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBoth no-net-change failure paths skip untracked-file rollback. Each site calls
run_git(repo_dir, "reset", "--hard", state.starting_head)directly instead of_abort_and_rollback.reset --hardrestores tracked state only, so untracked files created by the conflict resolver or the test adapter remain, and_application_resultstill reportsworktree_restored=True.scripts/backport/sweep.pythen continues the branch with those leftovers.
scripts/backport/candidate_apply.py#L564-L573: replace the plain reset with_abort_and_rollback(repo_dir, state.starting_head, run_git, run_process, state.starting_untracked_files)before returning_DIRTY_TREE_ERROR.scripts/backport/candidate_apply.py#L593-L604: apply the same replacement in the "nothing to commit" dirty-tree branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/candidate_apply.py` around lines 564 - 573, The no-net-change failure paths in candidate_apply.py#L564-L573 and candidate_apply.py#L593-L604 must roll back untracked files before returning _DIRTY_TREE_ERROR. Replace each direct run_git reset in the relevant candidate-application flow with _abort_and_rollback(repo_dir, state.starting_head, run_git, run_process, state.starting_untracked_files), preserving the existing _application_result behavior while ensuring worktree_restored is accurate.scripts/backport/git_commands.py (1)
100-114: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDistinguish a missing index stage from a failed
git show.
read_index_stagereturns""for every non-zero exit code. A transient Git failure is then indistinguishable from an empty blob.candidate_apply._apply_planstores this value inConflictedFile.target_branch_contentandsource_branch_content, and uses it for binary detection, so a failed read can present a conflicted file as empty content to the resolver.Consider raising for unexpected failures and returning
""only when the stage is known to be absent.♻️ Proposed change
def read_index_stage( repo_dir: str, path: str, stage: int, *, run_process: RunProcess = subprocess.run, ) -> str: result = run_process( ["git", "show", f":{stage}:{path}"], cwd=repo_dir, capture_output=True, text=True, errors="replace", ) - return result.stdout if result.returncode == 0 else "" + if result.returncode == 0: + return result.stdout + if not index_stage_exists(repo_dir, path, stage, run_process=run_process): + return "" + raise RuntimeError( + f"could not read index stage {stage} of {path}: " + + ((result.stderr or "").strip()[:300] or "git show failed") + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/backport/git_commands.py` around lines 100 - 114, Update read_index_stage to distinguish an absent index stage from other git show failures: return an empty string only for the expected missing-stage condition, and raise or otherwise propagate unexpected non-zero execution failures. Preserve successful stdout handling so candidate_apply._apply_plan receives accurate content for binary detection and conflict resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/backport/candidate_apply.py`:
- Around line 223-231: Update the applied-candidate handling around
_add_source_pr_trailer so result.resolved_commit_sha is always set to the
amended SHA after the trailer amend, regardless of result.resolved_by_ai.
Preserve the existing applied outcome flow and assign amended_sha for every
applied candidate before returning result.
In `@scripts/backport/mark_done.py`:
- Around line 84-85: Update the docstrings near the commit-detection logic to
reflect all supported rules: subject matching accepts trailing (`#N`), standard
GitHub merge subjects, and Backport-Source-PR trailers;
_applied_prs_from_commit_bodies also parses each full commit message in addition
to the ## Applied section. Keep the implementation unchanged.
In `@scripts/backport/source_plan.py`:
- Around line 63-72: Update the source_commits construction in
plan_source_change so that when source_commits_complete is false, the fetched
tip from _fetch_pr_head_tip is always removed from any existing position and
appended as the final commit. Preserve the existing commit order otherwise,
ensuring source_commits[-1] is consistently the PR head tip.
In `@tests/test_backport_missing_test_adaptation.py`:
- Around line 46-58: Configure commit.gpgsign=false for every real-Git fixture
repository before its first commit. In
tests/test_backport_missing_test_adaptation.py, add the setting after the
repository user.email configuration; in tests/test_backport_source_plan.py,
update the history fixture and each independently initialized repository at
lines 87-89, 143-145, 199-201, 217-219, 290-292, and 355-357, or centralize the
setup in a shared _init_repo helper.
---
Outside diff comments:
In `@scripts/backport/sweep_validation.py`:
- Around line 85-92: Update the docstring for the validation function returning
ValidationOutcome to document the actual return type and describe its
resolutions and ai_summary fields, replacing the outdated “(green, output)”
wording while preserving the existing repair behavior description.
---
Nitpick comments:
In `@scripts/backport/candidate_apply.py`:
- Around line 564-573: The no-net-change failure paths in
candidate_apply.py#L564-L573 and candidate_apply.py#L593-L604 must roll back
untracked files before returning _DIRTY_TREE_ERROR. Replace each direct run_git
reset in the relevant candidate-application flow with
_abort_and_rollback(repo_dir, state.starting_head, run_git, run_process,
state.starting_untracked_files), preserving the existing _application_result
behavior while ensuring worktree_restored is accurate.
In `@scripts/backport/git_commands.py`:
- Around line 100-114: Update read_index_stage to distinguish an absent index
stage from other git show failures: return an empty string only for the expected
missing-stage condition, and raise or otherwise propagate unexpected non-zero
execution failures. Preserve successful stdout handling so
candidate_apply._apply_plan receives accurate content for binary detection and
conflict resolution.
In `@scripts/backport/missing_test_adaptation.py`:
- Around line 442-466: Update invalid_sandbox_test_paths and its reporting flow
to distinguish deleted sandbox test files from other invalid generated test
paths. Track paths absent from sandbox_before separately, preserve the
fail-closed behavior for deletions, and emit a distinct sweep summary explaining
that files were removed.
In `@scripts/backport/pr_creator.py`:
- Around line 362-363: Update the AI Adaptation section condition in the
section-building logic to use any_ai_changes instead of ai_involved, matching
the existing Human Review Required condition and keeping both sections
consistent.
In `@scripts/backport/sweep.py`:
- Around line 463-476: Update the handling of validation_outcome in
validate_branch_with_optional_repair’s caller to access ValidationOutcome fields
directly: replace the getattr calls for resolutions and ai_summary with direct
attribute access, preserving the existing defaults only where they are
explicitly required by the surrounding logic.
- Around line 434-461: Capture the current HEAD SHA immediately before applying
each candidate, and use that captured SHA as the rollback target when validation
fails. Update the candidate application flow around candidate_apply and replace
the applied_commits-based rollback_ref calculation with a hard reset to the
pre-application SHA, preserving previously retained candidates regardless of how
many commits the candidate creates.
In `@tests/test_backport_main.py`:
- Around line 316-325: Rename the mock parameter corresponding to the patched
apply_candidate decorator from mock_executor_cls to mock_apply_candidate in
test_clean_cherry_pick_returns_success and the similarly affected tests at the
referenced locations, preserving decorator order and all other parameters.
In `@tests/test_backport_pr_creator.py`:
- Line 866: Update the assertion in the test covering build_pr_body to check for
the actual “Conflict Details” heading, asserting it is absent when
resolution_results is None. Remove the ineffective assertion against text that
build_pr_body never emits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 943084a7-1b5f-4929-84c6-74f7c577ce9d
📒 Files selected for processing (39)
.github/workflows/backport-mark-done-poll.yml.github/workflows/backport-poll.yml.github/workflows/backport-sweep.yml.github/workflows/backport.yml.github/workflows/release-notes-cut.ymldocs/architecture.mdexamples/repos.ymlrepos.ymlscripts/backport/candidate_apply.pyscripts/backport/cherry_pick.pyscripts/backport/diff_comments.pyscripts/backport/git_commands.pyscripts/backport/main.pyscripts/backport/mark_done.pyscripts/backport/matrix.pyscripts/backport/missing_test_adaptation.pyscripts/backport/models.pyscripts/backport/pr_creator.pyscripts/backport/registry.pyscripts/backport/revert_commit.pyscripts/backport/source_plan.pyscripts/backport/sweep.pyscripts/backport/sweep_apply.pyscripts/backport/sweep_git.pyscripts/backport/sweep_models.pyscripts/backport/sweep_reporting.pyscripts/backport/sweep_validation.pyscripts/backport/utils.pytests/test_backport_candidate_apply.pytests/test_backport_cherry_pick.pytests/test_backport_main.pytests/test_backport_mark_done.pytests/test_backport_missing_test_adaptation.pytests/test_backport_pr_creator.pytests/test_backport_source_plan.pytests/test_backport_sweep.pytests/test_diff_comments.pytests/test_matrix.pytests/test_registry.py
💤 Files with no reviewable changes (3)
- tests/test_backport_cherry_pick.py
- scripts/backport/cherry_pick.py
- scripts/backport/sweep_apply.py
Keep amended commit references valid, make candidate and validation rollback exact, and fail closed on Git index read errors. Clarify adaptation failures and review metadata while adding focused regression coverage for the reported edge cases. Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
1 participant
Summary
Fix cases where the backport workflow could apply an incomplete or incorrectly classified source change.
This PR combines the source-classification foundation and the shared application engine. They are being reviewed together because classification is only useful when the application path consumes it correctly.
Most of the diff is consolidation and test coverage, not new workflow behavior.
cherry_pick.pyandsweep_apply.py(963 lines).Correctness Fixes
Preserved behavior
Candidate discovery, ordering, batching, conflict resolution, missing-test adaptation, validation, rolling sweep PRs, human-controlled merges, and mark-done reconciliation remain unchanged.
For Valkey’s squash-and-merge workflow, the normal path remains one cherry-pick of GitHub’s squash commit.