fix(missions): judge local work against the remote tip, not the capture base
Mission 019fc476 lost phase 0's work again, and this time the cause was the interaction between two fixes I had just shipped. has_local_work compared HEAD against .git/clawmates-base to decide whether a checkout held mission work. advance_base_commit moves that marker to each phase's committed head. So the moment a phase committed successfully, base == HEAD, has_local_work reported "pristine", and the next phase's launch reset the work away. Phase 1 wrote CHAIN_MISSING.md. The preceding mission survived only because its phase 0 FAILED to commit and left a dirty tree. Fixing that failure is what exposed this one. One marker was carrying two meanings: "where should the next diff start" (rolling, per phase) and "is this checkout untouched" (fixed for the mission). Only the first belongs to clawmates-base. The second is now `origin/<branch>`, which does not move for the life of the mission, so a HEAD that differs from it means a phase committed — one commit ago or five. An unresolvable remote ref preserves, since wrongly skipping a refresh costs staleness while wrongly resetting destroys a phase. The existing test passed throughout because it never advanced the base. It now does, which makes it a reproduction rather than a restatement, and it needs a real bare origin to resolve origin/main the way a clone does. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
25d9805806
commit
1a979f500f
@@ -73,7 +73,7 @@ pub async fn ensure_checkout(
|
||||
// `ensure_checkout` runs at every phase launch, not once per mission.
|
||||
// Freshening a pristine checkout is right; freshening one that already
|
||||
// holds this mission's work destroys it. See `has_local_work`.
|
||||
if has_local_work(&path) {
|
||||
if has_local_work(&path, default_branch) {
|
||||
eprintln!(
|
||||
"mission_workspace: {} already holds mission work — skipping \
|
||||
fetch/reset so earlier phases' output survives",
|
||||
@@ -159,7 +159,7 @@ async fn clone(path: &std::path::Path, url: &str) -> Result<(), String> {
|
||||
/// tree. Both are checked because the two phases of the failure look different:
|
||||
/// an agent that committed leaves a clean tree at a new HEAD, and one that did
|
||||
/// not leaves a dirty tree at the old HEAD.
|
||||
fn has_local_work(path: &std::path::Path) -> bool {
|
||||
fn has_local_work(path: &std::path::Path, branch: &str) -> bool {
|
||||
let git = |args: &[&str]| -> Option<String> {
|
||||
let out = std::process::Command::new("git")
|
||||
.arg("-C")
|
||||
@@ -180,13 +180,29 @@ fn has_local_work(path: &std::path::Path) -> bool {
|
||||
}
|
||||
}
|
||||
|
||||
// Otherwise compare HEAD against the base recorded when the checkout was
|
||||
// made. A HEAD that has moved means a phase committed. If the base was
|
||||
// never recorded we cannot tell, and the safe answer is to preserve:
|
||||
// wrongly skipping a refresh costs staleness, wrongly resetting costs a
|
||||
// Otherwise compare HEAD against the *remote tip*, which is the only
|
||||
// fixed point here.
|
||||
//
|
||||
// This deliberately does not use `.git/clawmates-base`. That marker is the
|
||||
// rolling capture base and `advance_base_commit` moves it to each phase's
|
||||
// committed head — so comparing HEAD against it asks "did anything happen
|
||||
// since the last commit we made", which is false immediately after every
|
||||
// successful delivery. Mission `019fc476` lost phase 0's file exactly that
|
||||
// way: phase 0 committed, the base advanced to match HEAD, and phase 1's
|
||||
// launch concluded the checkout was pristine and reset it. The preceding
|
||||
// mission survived only because its phase 0 *failed* to commit and left a
|
||||
// dirty tree.
|
||||
//
|
||||
// `origin/<branch>` does not move for the life of the mission, so "HEAD is
|
||||
// not the remote tip" means a phase committed, whether one commit ago or
|
||||
// five. If the remote ref cannot be resolved the answer is preserve:
|
||||
// wrongly skipping a refresh costs staleness, wrongly resetting destroys a
|
||||
// phase's output.
|
||||
match (base_commit(path), git(&["rev-parse", "HEAD"])) {
|
||||
(Some(base), Some(head)) => base != head,
|
||||
match (
|
||||
git(&["rev-parse", &format!("origin/{branch}")]),
|
||||
git(&["rev-parse", "HEAD"]),
|
||||
) {
|
||||
(Some(tip), Some(head)) => tip != head,
|
||||
_ => true,
|
||||
}
|
||||
}
|
||||
@@ -607,7 +623,15 @@ mod tests {
|
||||
assert!(body.contains("/AGENTS.md"));
|
||||
}
|
||||
|
||||
fn seed(dir: &std::path::Path) {
|
||||
/// Seed a checkout that has an `origin`, like a real clone does. Without
|
||||
/// one `origin/<branch>` does not resolve and `has_local_work` takes its
|
||||
/// preserve-by-default path, which would make the pristine case untestable.
|
||||
fn seed(dir: &std::path::Path, remote: &std::path::Path) {
|
||||
std::process::Command::new("git")
|
||||
.args(["init", "--quiet", "--bare"])
|
||||
.arg(remote)
|
||||
.output()
|
||||
.unwrap();
|
||||
let g = |args: &[&str]| {
|
||||
std::process::Command::new("git")
|
||||
.arg("-C")
|
||||
@@ -619,47 +643,44 @@ mod tests {
|
||||
g(&["init", "--quiet"]);
|
||||
g(&["config", "user.email", "[email protected]"]);
|
||||
g(&["config", "user.name", "T"]);
|
||||
g(&["checkout", "-q", "-B", "main"]);
|
||||
std::fs::write(dir.join("README.md"), "# base\n").unwrap();
|
||||
g(&["add", "."]);
|
||||
g(&["commit", "--quiet", "-m", "base"]);
|
||||
g(&["remote", "add", "origin", &remote.display().to_string()]);
|
||||
g(&["push", "--quiet", "origin", "main"]);
|
||||
g(&["fetch", "--quiet", "origin", "main"]);
|
||||
record_base_commit(dir);
|
||||
}
|
||||
|
||||
/// A checkout mid-mission must not be mistaken for a cold one.
|
||||
///
|
||||
/// `ensure_checkout` runs per phase launch and resets on the reuse path.
|
||||
/// Mission `019fc444` lost phase 0's committed file that way — phase 1
|
||||
/// started from `origin/main` and never saw it. These are the three states
|
||||
/// that path has to tell apart.
|
||||
/// Mission `019fc444` lost phase 0's committed file that way. The fix then
|
||||
/// failed again on mission `019fc476` for a different reason, which the
|
||||
/// last case here pins down.
|
||||
#[test]
|
||||
fn local_work_is_recognized_before_a_checkout_is_reset() {
|
||||
let tmp = tempfile::tempdir().unwrap();
|
||||
let repo = tmp.path();
|
||||
seed(repo);
|
||||
let repo = &tmp.path().join("repo");
|
||||
std::fs::create_dir_all(repo).unwrap();
|
||||
seed(repo, &tmp.path().join("remote.git"));
|
||||
let repo = repo.as_path();
|
||||
|
||||
assert!(
|
||||
!has_local_work(repo),
|
||||
!has_local_work(repo, "main"),
|
||||
"a freshly cloned checkout has no work and may be refreshed"
|
||||
);
|
||||
|
||||
// An agent that wrote files and did not commit: dirty tree, HEAD put.
|
||||
std::fs::write(repo.join("ALPHA.md"), "ALPHA\n").unwrap();
|
||||
assert!(has_local_work(repo), "uncommitted agent output is work");
|
||||
assert!(has_local_work(repo, "main"), "uncommitted agent output is work");
|
||||
|
||||
// An agent (or delivery) that committed: clean tree, HEAD moved. This
|
||||
// is the shape that was actually being destroyed, because a hard reset
|
||||
// is the shape that was destroyed on 019fc444, because a hard reset
|
||||
// leaves untracked files alone but removes tracked ones.
|
||||
for args in [
|
||||
vec!["add", "ALPHA.md"],
|
||||
vec!["commit", "--quiet", "-m", "phase 0"],
|
||||
] {
|
||||
std::process::Command::new("git")
|
||||
.arg("-C")
|
||||
.arg(repo)
|
||||
.args(&args)
|
||||
.output()
|
||||
.unwrap();
|
||||
}
|
||||
git_in(repo, &["add", "ALPHA.md"]);
|
||||
git_in(repo, &["commit", "--quiet", "-m", "phase 0"]);
|
||||
let status = std::process::Command::new("git")
|
||||
.arg("-C")
|
||||
.arg(repo)
|
||||
@@ -671,8 +692,41 @@ mod tests {
|
||||
"the commit left a clean tree — the case a dirty-tree check misses"
|
||||
);
|
||||
assert!(
|
||||
has_local_work(repo),
|
||||
has_local_work(repo, "main"),
|
||||
"committed phase output must not be reset away"
|
||||
);
|
||||
|
||||
// The regression from 019fc476. Delivery advances the capture base to
|
||||
// the commit it just made, so any check comparing HEAD against that
|
||||
// base reports "nothing happened" the instant a phase succeeds — and
|
||||
// the next phase resets the work away. Advancing it here is what makes
|
||||
// this a real reproduction rather than a restatement of the case above.
|
||||
let head = std::process::Command::new("git")
|
||||
.arg("-C")
|
||||
.arg(repo)
|
||||
.args(["rev-parse", "HEAD"])
|
||||
.output()
|
||||
.unwrap();
|
||||
let head = String::from_utf8_lossy(&head.stdout).trim().to_string();
|
||||
advance_base_commit(repo, &head);
|
||||
assert_eq!(
|
||||
base_commit(repo).as_deref(),
|
||||
Some(head.as_str()),
|
||||
"the base now equals HEAD, which is the trap"
|
||||
);
|
||||
assert!(
|
||||
has_local_work(repo, "main"),
|
||||
"a phase that committed successfully must still count as work \
|
||||
after the capture base advances to match its commit"
|
||||
);
|
||||
}
|
||||
|
||||
fn git_in(dir: &std::path::Path, args: &[&str]) {
|
||||
std::process::Command::new("git")
|
||||
.arg("-C")
|
||||
.arg(dir)
|
||||
.args(args)
|
||||
.output()
|
||||
.unwrap();
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user