Conversation
There was a problem hiding this comment.
Pull request overview
Adds an --abort escape hatch to dgr sync so users can cancel a paused restack operation by aborting any in-progress git rebase and clearing operation.json.
Changes:
- Introduces
--abortflag fordgr sync, with mutual exclusion vs--continue. - Implements core abort flow (
abort_sync) and a git helper (git rebase --abort). - Updates the “pending operation” error message to suggest
--abortas an alternative to--continue.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/core/workflow.rs | Updates pending-operation error message to mention dgr sync --abort. |
| src/core/sync.rs | Adds abort_operation option and abort_sync() implementation. |
| src/core/git.rs | Adds abort_rebase() helper that runs git rebase --abort. |
| src/cli/sync/mod.rs | Adds --abort CLI flag, enforces conflicts with --continue, prints success message, updates CLI→core option conversion and tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[derive(Debug, Clone, Default, PartialEq, Eq)] | ||
| pub struct SyncOptions { | ||
| pub continue_operation: bool, | ||
| pub abort_operation: bool, | ||
| } |
There was a problem hiding this comment.
Adding abort_operation to SyncOptions requires updating all SyncOptions { ... } struct literals. There is at least one existing literal later in this file (in the tests) that now omits abort_operation, which will not compile. Consider either adding the field at that call site(s) or switching those literals to ..Default::default() to make future option additions non-breaking.
There was a problem hiding this comment.
Addressed — verified no other SyncOptions literals exist in the codebase besides the test, which already has the field. Added the missing abort_operation: false to the test struct literal. The other usages all go through SyncOptions::default() which handles it via the derived Default impl.
There was a problem hiding this comment.
Fixed in b5801c8. All SyncOptions struct literals in production and test code now include the abort_operation field. The struct derives Default, so the three call sites using SyncOptions::default() correctly get abort_operation: false. The one explicit struct literal (in the resume test) already had the field from the prior commit. The new abort tests also use explicit struct literals with both fields.
|
|
||
| fn abort_sync() -> io::Result<SyncOutcome> { | ||
| let session = open_initialized("dagger is not initialized; run 'dgr init' first")?; | ||
| let pending_operation = load_operation(&session.paths)? |
There was a problem hiding this comment.
pending_operation is loaded but never used in abort_sync, which will trigger an unused-variable warning. If warnings are treated as errors in CI, this will break the build. Rename it to _pending_operation or use it (e.g., include the origin in the abort output / error handling).
| let pending_operation = load_operation(&session.paths)? | |
| let _pending_operation = load_operation(&session.paths)? |
There was a problem hiding this comment.
Addressed in the latest push — renamed to _pending_operation.
There was a problem hiding this comment.
Fixed in b5801c8. Replaced the unused _pending_operation variable binding with a direct is_none() check: if load_operation(&session.paths)?.is_none() { return Err(...); }. This validates that a pending operation exists before proceeding (returning a clear error if there is nothing to abort) without binding an unused variable.
| fn abort_sync() -> io::Result<SyncOutcome> { | ||
| let session = open_initialized("dagger is not initialized; run 'dgr init' first")?; | ||
| let pending_operation = load_operation(&session.paths)? | ||
| .ok_or_else(|| io::Error::other("no paused dgr operation to abort"))?; | ||
|
|
||
| if git::is_rebase_in_progress(&session.repo) { | ||
| let abort_output = git::abort_rebase()?; | ||
| if !abort_output.status.success() { | ||
| return Ok(SyncOutcome { |
There was a problem hiding this comment.
The new --abort/abort_sync path doesn’t appear to have coverage in the existing src/core/sync.rs test module (which already exercises --continue resume behavior). Add tests for: (1) erroring when there’s no paused operation, (2) aborting an in-progress rebase, and (3) clearing operation.json on success.
There was a problem hiding this comment.
Good point. Integration test coverage for abort requires a full conflicting restack setup. Added as a follow-up TODO. The core abort logic is straightforward (check pending op, abort rebase, clear operation file) and is covered by the existing operation persistence tests for the underlying primitives.
There was a problem hiding this comment.
Fixed in b5801c8. Added two tests for the abort path:
-
abort_cancels_paused_sync_and_clears_pending_operation-- sets up a real conflict (divergentshared.txton parent and child branches), verifies sync pauses with a pending operation, then calls abort and asserts: the outcome succeeds,pausedis false, and the pending operation file is cleared. -
abort_returns_error_when_no_pending_operation-- calls abort on an initialized repo with no paused operation and verifies it returns the expected error message.
Adds Dependabot security-only config, auto-merge workflow, and dependency audit CI.
don-petry
left a comment
don-petry
left a comment
There was a problem hiding this comment.
Automated review — NEEDS HUMAN REVIEW
Risk: MEDIUM
Reviewed commit: b5801c85fc6b47c7dbb70504a140368ea97ccd0f
Council vote: security=LOW · correctness=MEDIUM · maintainability=MEDIUM
Summary
This PR cleanly implements dgr sync --abort to address issue #10 item 5, with sound core logic and integration tests covering the primary success and error paths. However, two blockers prevent merging: the branch is in a CONFLICTING/DIRTY merge state that must be resolved, and there are no CI check results recorded against the head SHA, making it impossible to verify that the required cargo check/cargo test baseline passes. Two minor issues also warrant attention — a misleading failure message when rebase-abort fails, and two unchecked test-plan items.
Linked issue analysis
Issue #10 item 5 ("No --abort for paused operations") is well-addressed: --abort flag, mutual exclusion with --continue, pending-operation guard, conditional rebase abort, clear_operation cleanup, and updated error message all match the stated requirements. closingIssuesReferences is empty because the PR intentionally targets only one of the six items in the issue.
Findings
Major
- [major] (correctness + maintainability) — PR has
mergeable=CONFLICTING/mergeStateStatus=DIRTY. The branch has fallen behindmainand has unresolved conflicts; must be rebased or merged before this PR can land. - [major] (correctness) —
statusCheckRollupis empty: no CI runs are recorded against head SHAb5801c85. AGENTS.md specifiescargo checkandcargo testas the default verification baseline; there is no evidence these passed.
Minor
- [minor] (correctness)
src/core/sync.rs:322— Inabort_sync(), whengit::abort_rebase()returns a non-success status, the function returnsSyncOutcome { paused: true, ... }. The CLI's downstream handler then callsprint_restack_pause_guidance(), which tells the user to rundgr sync --continue— the opposite of what they just tried. The failure should be surfaced differently (e.g.paused: falseor a dedicated error variant). - [minor] (correctness + maintainability) — Two test-plan checklist items remain unchecked: (1)
dgr sync --continue --abortis rejected by clap as conflicting flags, and (2)dgr sync --abortprints "Aborted paused restack operation." on success. The success-message path is exercisable and worth a CLI-level assertion.
Info
- [info] (security)
src/core/git.rs:237—abort_rebase()usesCommand::new("git").args(["rebase", "--abort"])— safe pattern, no user-input interpolation, no shell=true. - [info] (security)
src/core/sync.rs:319— Minor theoretical TOCTOU betweenload_operation()check andclear_operation()call inabort_sync(). Not exploitable in practice (single-user CLI, operation file is in local.git). Already tracked in issue #10 item 1. - [info] (correctness) — Issue #10 item 5 is well-addressed by this diff. The PR intentionally addresses only one of the six items in the issue.
- [info] (correctness)
src/core/sync.rs:1682— The abort-with-rebase-failure path (wheregit::abort_rebasereturns non-success) has no test coverage. - [info] (maintainability)
src/core/sync.rs:312—abort_sync()callsopen_initialized()before checking for a pending operation, sodgr sync --aborton an uninitialised repo returns "dagger is not initialized" rather than the more intuitive "no paused dgr operation to abort". Minor UX inconsistency. - [info] (maintainability)
src/core/sync.rs:1700— The abort integration test callscrate::core::store::open_initialized()directly to inspect internal state. Acceptable per AGENTS.md's smoke-test guidance and consistent with similar tests in the file.
CI status
statusCheckRollup is empty — no CI checks are configured or recorded against this repository. Cannot verify cargo check/cargo test pass for this commit.
Reviewed automatically by the don-petry PR-review council (security: opus 4.6 · correctness: sonnet 4.6 · maintainability: sonnet 4.6 · synthesis: sonnet 4.6). The marker on line 1 lets the agent detect new commits and re-review. Reply with @don-petry if you need a human.
When a restack pauses on a conflict, users had to manually run git rebase --abort and clean up operation.json. This adds dgr sync --abort which safely aborts any in-progress git rebase and clears the pending operation state. The flag is mutually exclusive with --continue via clap's conflicts_with. The pending operation error message now mentions --abort as an option alongside --continue. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Prefix unused `pending_operation` variable with underscore - Add missing `abort_operation` field to SyncOptions struct literal in test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unused variable binding in abort_sync; use is_none() check directly for clearer control flow - Confirm all SyncOptions struct literals include abort_operation field (all existing uses already covered via Default or explicit fields) - Add test coverage for abort_sync: verify abort clears pending operation after a paused sync, and returns an error when no pending operation exists Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When git rebase --abort fails, returning paused=true caused the CLI to show misleading --continue guidance. Now returns paused=false so the generic error path displays the actual failure output instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
don-petry
force-pushed
the
feat/sync-abort
branch
from
b5801c8 to
6961c26
Compare
don-petry
left a comment
don-petry
left a comment
There was a problem hiding this comment.
Automated review — NEEDS HUMAN REVIEW
Risk: MEDIUM
Reviewed commit: 6961c260ea98a8bcd554912f02f42fb18a6bdf81
Cascade: triage(haiku) → sonnet
Summary
New commit (6961c26) correctly fixes the prior-review minor finding by returning paused=false when git rebase --abort fails, eliminating the misleading --continue guidance. Core logic and tests are sound. Two hard gates still block merge: the branch is CONFLICTING/DIRTY and must be rebased onto main, and statusCheckRollup is empty so there is no CI evidence that cargo check/cargo test pass against the current head SHA.
Findings
Major
- [major]
(branch)— Branch is mergeable=CONFLICTING / mergeStateStatus=DIRTY. The branch has fallen behind main with unresolved conflicts and cannot be merged until rebased or the conflicts are resolved. - [major]
(CI)— statusCheckRollup is empty — no CI runs recorded against head SHA 6961c26. Cannot verify cargo check / cargo test pass for this commit.
Minor
-
[minor]
.github/workflows/dependabot-automerge.yml:43— dependabot-automerge.yml uses pull_request_target (a privileged trigger) and interpolates ${{ steps.metadata.outputs.update-type }}, ${{ steps.metadata.outputs.dependency-type }}, and ${{ steps.metadata.outputs.package-ecosystem }} directly into a bash run: script. Although Dependabot metadata is not user-controlled, the pattern is a known GitHub Actions security smell. Prefer passing these values via env: to avoid interpolation into shell. -
[minor]
src/cli/sync/mod.rs— Two test-plan checklist items remain unchecked: (1)dgr sync --continue --abortis rejected by clap as conflicting flags — this is enforced statically by clap but lacks an automated assertion; (2)dgr sync --abortprints "Aborted paused restack operation." on success — the CLI success-message path has no test coverage.
Info
- [info]
src/core/sync.rs:322— New commit (6961c26) correctly addresses the prior-review minor finding: abort_sync() now returns paused=false when git::abort_rebase() fails, so the CLI shows the actual failure output rather than misleading --continue guidance. - [info]
src/core/git.rs:237— abort_rebase() uses Command::new("git").args(["rebase", "--abort"]) — safe pattern, no user-input interpolation, no shell=true. - [info]
.github/workflows/dependency-audit.yml— dependency-audit.yml pins all action SHAs (actions/checkout, actions/setup-node, dtolnay/rust-toolchain, etc.) which is good security hygiene. - [info]
.github/workflows/dependency-audit.yml:175— audit-cargo step runs "cargo generate-lockfile 2>/dev/null; cargo audit" — the 2>/dev/null silently discards lockfile-generation errors. This is minor but could mask problems in CI. - [info]
src/core/sync.rs:313— abort_sync() calls open_initialized() before checking for a pending operation, sodgr sync --aborton an uninitialised repo returns "dagger is not initialized" rather than "no paused dgr operation to abort". Minor UX inconsistency, carried over from prior review. - [info]
src/core/sync.rs:319— Minor theoretical TOCTOU between load_operation() check and clear_operation() call in abort_sync(). Not exploitable in practice (single-user CLI, operation file is in local .git). Noted in prior review.
CI status
No CI runs recorded against head SHA 6961c26 (statusCheckRollup is empty). CI must pass before merge.
Reviewed by the don-petry PR-review cascade (triage: haiku 4.5 → deep: sonnet 4.6 → audit: opus 4.6). Reply with @don-petry if you need a human.
Why?
When
dgr synchits a merge conflict, the user resolves it and runs--continue. But sometimes the user decides they don't want to proceed — the conflict is too complex, or they want to take a different approach. Without--abort, the only option is manually runninggit rebase --abortand deletingoperation.json, which is error-prone and undocumented.Summary
--abortflag todgr syncthat safely cancels a paused restack operation by runninggit rebase --abort(if a rebase is in progress) and clearingoperation.json--continueand--abortmutually exclusive via clap'sconflicts_with--abortas an option alongside--continueAddresses item 5 from #10.
Test plan
dgr sync --abortwith no paused operation errors with "no paused dgr operation to abort"dgr sync --abortafter a conflict aborts the rebase and clears operation.jsondgr sync --continue --abortis rejected by clap as conflicting flagsdgr sync --abortprints "Aborted paused restack operation." on success🤖 Generated with Claude Code