Issue #10: resume intent log + compensator preflight - #13
Conversation
|
Addressed the review findings in 9b6c59d:\n\n1. is now preserved across re-attempts (only increments).\n2. Control-DAG intent-write failures now return explicit failed outcomes (no silent propagation).\n3. resume idempotency guard now requires merged+divergent history and clean worktree; fresh branches at are no longer treated as already-landed.\n4. Outcome persistence failures now emit explicit warnings instead of being silently discarded.\n\nAlso added regression tests for each behavior and bumped version to . All checks are green locally (, , , ). |
|
Addressed the review findings in 9b6c59d:
Also added regression tests for each behavior and bumped version to |
📝 WalkthroughWalkthroughAdds persistent per-step intent and outcome logging plus compensator-plan generation for workflow resume; threads run/workflow identifiers through execution paths, introduces idempotency guards for Git/worktree-based steps, and surfaces failed step indices in ResumeContext. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as "cli/run.rs"
participant Executor as "automation/mod.rs"
participant State as "state/mod.rs"
participant Git as "Git checks / worktree"
User->>CLI: request resume run
CLI->>Executor: resolve workflow + resume context
CLI->>Executor: request compensator plan -> Executor
Executor->>State: load intent for step N
alt intent exists and no successful outcome
State-->>Executor: WorkflowStepIntentRecord
Executor->>Git: check idempotency (branch merged? PR exists? worktree changes?)
alt side-effects present
Git-->>Executor: detected prior side-effects
Executor-->>CLI: include compensator actions in plan / skip replay
else
Git-->>Executor: no side-effects found
Executor->>Executor: schedule step, record intent
Executor->>Executor: execute step
Executor->>State: record outcome
end
else no intent
Executor->>Executor: record intent, execute step
Executor->>State: record outcome
end
Executor-->>CLI: return compensator plan
CLI-->>User: print resume plan
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Cargo.toml (1)
3-3: Version discrepancy with PR summary.The PR summary mentions bumping version to 0.2.1, but the actual version is 0.2.2. Please verify the intended version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` at line 3, The Cargo.toml entry version = "0.2.2" does not match the PR summary which states 0.2.1; decide the intended release and make them consistent by updating the Cargo.toml version field (version = "...") to the correct value or update the PR summary/release notes to reflect version = "0.2.2".src/automation/mod.rs (2)
2662-2691: Duplication of idempotency guard logic.This land idempotency guard duplicates the logic at lines 1403-1432. Consider extracting a shared helper function (e.g.,
should_skip_land_replay) to ensure both code paths stay consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/automation/mod.rs` around lines 2662 - 2691, The idempotency guard that checks prior_unsuccessful_intent duplicates logic elsewhere; extract this into a shared helper (e.g., should_skip_land_replay) and call it from both locations. Implement should_skip_land_replay(project_root, agent, prior_unsuccessful_intent) to encapsulate the agent_branch lookup, is_branch_merged, branch_has_divergent_commits, and worktree_has_changes checks and return a bool plus any message needed; then replace the duplicated block in the current function (the if let Some(prior_unsuccessful_intent) { ... } block) with a call to that helper and construct the ControlStepOutcome/StepResult only when the helper indicates skipping. Ensure the helper uses the same semantics/wrapping of unwrap_or defaults as the original checks to keep behavior identical.
2163-2170: Consider reusing existing worktree inspection logic.The
worktree_has_changesfunction duplicates logic fromsrc/worktree/mod.rs::inspect_worktree()(see relevant code snippet 2). While acceptable for this PR, consider consolidating these utilities in a future refactor to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/automation/mod.rs` around lines 2163 - 2170, worktree_has_changes duplicates logic from src/worktree/mod.rs::inspect_worktree; replace the duplicated implementation by calling inspect_worktree for the same path and mapping its result to a bool, instead of re-running git_output_for_automation here. Import or reference inspect_worktree, compute worktree_path = project_root.join(".tutti").join("worktrees").join(agent), call inspect_worktree(&worktree_path) and return Ok(true/false) based on whether it reports changes, propagating errors as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/automation/mod.rs`:
- Around line 1403-1432: The idempotency guard currently requires divergent ==
true which misses fast-forward merges; update the conditional in the
prior_unsuccessful_intent handling (the block using agent_branch,
is_branch_merged, branch_has_divergent_commits, worktree_has_changes) to skip
replay when the branch is merged and the worktree is clean regardless of
divergence (i.e., remove the divergent check), and adjust/remove the now-unused
branch_has_divergent_commits result or its call accordingly; keep the StepResult
construction (step_results push with message about already merged) and ensure
the message still reports branch and intent.planned_at.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 3: The Cargo.toml entry version = "0.2.2" does not match the PR summary
which states 0.2.1; decide the intended release and make them consistent by
updating the Cargo.toml version field (version = "...") to the correct value or
update the PR summary/release notes to reflect version = "0.2.2".
In `@src/automation/mod.rs`:
- Around line 2662-2691: The idempotency guard that checks
prior_unsuccessful_intent duplicates logic elsewhere; extract this into a shared
helper (e.g., should_skip_land_replay) and call it from both locations.
Implement should_skip_land_replay(project_root, agent,
prior_unsuccessful_intent) to encapsulate the agent_branch lookup,
is_branch_merged, branch_has_divergent_commits, and worktree_has_changes checks
and return a bool plus any message needed; then replace the duplicated block in
the current function (the if let Some(prior_unsuccessful_intent) { ... } block)
with a call to that helper and construct the ControlStepOutcome/StepResult only
when the helper indicates skipping. Ensure the helper uses the same
semantics/wrapping of unwrap_or defaults as the original checks to keep behavior
identical.
- Around line 2163-2170: worktree_has_changes duplicates logic from
src/worktree/mod.rs::inspect_worktree; replace the duplicated implementation by
calling inspect_worktree for the same path and mapping its result to a bool,
instead of re-running git_output_for_automation here. Import or reference
inspect_worktree, compute worktree_path =
project_root.join(".tutti").join("worktrees").join(agent), call
inspect_worktree(&worktree_path) and return Ok(true/false) based on whether it
reports changes, propagating errors as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d2a4e35c-308c-41f9-95cf-01b048692298
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlsrc/automation/mod.rssrc/cli/run.rssrc/state/mod.rs
|
Addressed the requested changes in ea3ac34:
Validation rerun clean:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/automation/mod.rs (1)
2088-2130: Consider using creation time instead of modification time for robustness.The function uses
meta.modified()to determine if a review packet was created after the intent timestamp. While modification time works for most cases, it could theoretically be updated by file operations other than creation (e.g., reads on some filesystems, or accidental touches).If the platform supports it,
meta.created()would be more semantically correct. However, this is a minor concern since modification time is standard practice and the current implementation is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/automation/mod.rs` around lines 2088 - 2130, The code in review_packet_exists_since uses file modification time (meta.modified()) to decide if a review packet is "since" the given timestamp; change this to prefer creation time when available by calling meta.created() and falling back to meta.modified() if created() returns Err or is unsupported, update the variable names (e.g., created_or_modified -> modified_time) and comparisons where modified is used (the tuple in best: Option<(std::time::SystemTime, PathBuf)> and the is_none_or closure) so the function prefers the creation timestamp but retains modification-time behavior on platforms that don't expose creation time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/automation/mod.rs`:
- Around line 2088-2130: The code in review_packet_exists_since uses file
modification time (meta.modified()) to decide if a review packet is "since" the
given timestamp; change this to prefer creation time when available by calling
meta.created() and falling back to meta.modified() if created() returns Err or
is unsupported, update the variable names (e.g., created_or_modified ->
modified_time) and comparisons where modified is used (the tuple in best:
Option<(std::time::SystemTime, PathBuf)> and the is_none_or closure) so the
function prefers the creation timestamp but retains modification-time behavior
on platforms that don't expose creation time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0e42cb21-10e3-4ba4-90d4-e41dd8c1c19b
📒 Files selected for processing (1)
src/automation/mod.rs
1 participant
Summary
.tutti/state/workflow-intents/<run_id>/<step_id>.jsontt run --resume)land: skip replay when branch is already merged and the agent worktree is cleanreview: skip replay if a review packet already exists since prior attemptensure_running: keeps existing idempotent no-op behavior0.2.2Tests
cargo fmt,cargo clippy --all-targets --all-features -- -D warnings,cargo test,cargo build --release)Closes #10
Summary by CodeRabbit
New Features
Bug Fixes
Chores