fix(missions): capture from the clone point, and let agents create files
Two defects found by running a real coding mission (019fc372) rather than a test. Both made a coding phase look like it produced nothing. **Capture measured the wrong baseline.** It diffed the working tree against HEAD, which is correct only while work stays uncommitted. `rust_sdlc` has a *committer* role, so committing is the intended path — meaning a mission that did its job properly leaves a clean tree and captured nothing. That is exactly what happened: the agent created `DELIVERY_PROBE.md`, committed it as `aa3be95`, and the artifact recorded `empty: true` beside a commit that plainly contained the work. `mission_workspace` now records the clone point in `.git/clawmates-base` (in `.git/`, so it travels with the checkout, stays invisible to the repository, and cannot be reached by an agent through its pinned workspace), refreshed whenever `fetch_and_reset` moves HEAD. Capture diffs from there, covering committed, staged and unstaged changes in one pass. Checkouts predating the marker fall back to HEAD and say so via `base_recorded: false`. **Agents could not create files.** `coding_readwrite` granted `file_edit` but not `file_write`. `file_edit` replaces an exact existing string and rejects an empty `old_string`, so creating a new file was impossible. The mission transcript is unambiguous: "the tool rejected empty old_string... the shell is restricted", after which the agent worked around it through `shell`. The comment above that profile has claimed it grants file_write since the day it was written; the list never contained it. Also broadens capture from coding/benchmark/security_scan to every phase kind of a repo-bearing mission: `phase_task_text` tells research phases to "save findings under /mission/repo/research/", so filtering by kind would have discarded every research brief such a mission produced. Regression tests cover committed-only and committed-plus-uncommitted work against a real git repo. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
322c1be89c
commit
409ca65ee7
@@ -120,10 +120,28 @@ pub async fn capture_phase_diff_at(
|
|||||||
return Ok(None);
|
return Ok(None);
|
||||||
}
|
}
|
||||||
|
|
||||||
let base_sha = git(&repo, &["rev-parse", "HEAD"])
|
// Diff from where the mission *started*, not from HEAD.
|
||||||
|
//
|
||||||
|
// `HEAD` is the wrong baseline the moment an agent commits, and committing
|
||||||
|
// is the normal path — `rust_sdlc` has a committer role. A mission that did
|
||||||
|
// its job properly leaves a clean tree, so a HEAD-relative diff reports
|
||||||
|
// nothing changed. That is exactly what happened on mission 019fc372: the
|
||||||
|
// agent committed the file it was asked to create and capture recorded
|
||||||
|
// `empty: true` beside a commit that plainly contained the work.
|
||||||
|
//
|
||||||
|
// Diffing against the recorded clone point covers committed, staged and
|
||||||
|
// unstaged changes in one pass. Falling back to HEAD keeps checkouts made
|
||||||
|
// before the base was recorded working, at the cost of missing committed
|
||||||
|
// work — which is why the fallback says so in the metadata.
|
||||||
|
let recorded_base = mission_workspace::base_commit(&repo);
|
||||||
|
let base_is_recorded = recorded_base.is_some();
|
||||||
|
let base_sha = match recorded_base {
|
||||||
|
Some(sha) => sha,
|
||||||
|
None => git(&repo, &["rev-parse", "HEAD"])
|
||||||
.await
|
.await
|
||||||
.map(|s| s.trim().to_string())
|
.map(|s| s.trim().to_string())
|
||||||
.unwrap_or_else(|_| "unknown".to_string());
|
.unwrap_or_else(|_| "HEAD".to_string()),
|
||||||
|
};
|
||||||
|
|
||||||
// `--intent-to-add` registers untracked files with the index without
|
// `--intent-to-add` registers untracked files with the index without
|
||||||
// staging their content, which is what makes them appear in `git diff`.
|
// staging their content, which is what makes them appear in `git diff`.
|
||||||
@@ -139,11 +157,11 @@ pub async fn capture_phase_diff_at(
|
|||||||
// A repo with nothing to add is fine; keep going and let the diff be empty.
|
// A repo with nothing to add is fine; keep going and let the diff be empty.
|
||||||
let _ = git(&repo, &add).await;
|
let _ = git(&repo, &add).await;
|
||||||
|
|
||||||
let mut diff_args = vec!["diff", "HEAD", "--"];
|
let mut diff_args = vec!["diff", base_sha.as_str(), "--"];
|
||||||
diff_args.extend(excludes.iter().map(String::as_str));
|
diff_args.extend(excludes.iter().map(String::as_str));
|
||||||
let patch = git(&repo, &diff_args).await.unwrap_or_default();
|
let patch = git(&repo, &diff_args).await.unwrap_or_default();
|
||||||
|
|
||||||
let mut stat_args = vec!["diff", "HEAD", "--stat", "--"];
|
let mut stat_args = vec!["diff", base_sha.as_str(), "--stat", "--"];
|
||||||
stat_args.extend(excludes.iter().map(String::as_str));
|
stat_args.extend(excludes.iter().map(String::as_str));
|
||||||
let diffstat = git(&repo, &stat_args).await.unwrap_or_default();
|
let diffstat = git(&repo, &stat_args).await.unwrap_or_default();
|
||||||
|
|
||||||
@@ -175,6 +193,7 @@ pub async fn capture_phase_diff_at(
|
|||||||
|
|
||||||
let meta = json!({
|
let meta = json!({
|
||||||
"base_sha": base_sha,
|
"base_sha": base_sha,
|
||||||
|
"base_recorded": base_is_recorded,
|
||||||
"files_changed": files_changed,
|
"files_changed": files_changed,
|
||||||
"insertions": insertions,
|
"insertions": insertions,
|
||||||
"deletions": deletions,
|
"deletions": deletions,
|
||||||
|
|||||||
@@ -121,9 +121,58 @@ async fn clone(path: &std::path::Path, url: &str) -> Result<(), String> {
|
|||||||
}
|
}
|
||||||
scrub_remote_credentials(path, url);
|
scrub_remote_credentials(path, url);
|
||||||
ignore_agent_scaffolding(path);
|
ignore_agent_scaffolding(path);
|
||||||
|
record_base_commit(path);
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Remember the commit the mission started from.
|
||||||
|
///
|
||||||
|
/// Delivery needs to answer "what did this mission change", and the obvious
|
||||||
|
/// reading — working tree versus `HEAD` — is wrong the moment an agent
|
||||||
|
/// commits. `rust_sdlc` has a *committer* role, so committing is the normal
|
||||||
|
/// path, not an edge case: a mission that did its job properly would have a
|
||||||
|
/// clean tree and capture nothing at all. Observed exactly that on mission
|
||||||
|
/// 019fc372, where the agent committed `DELIVERY_PROBE.md` and the diff came
|
||||||
|
/// back empty.
|
||||||
|
///
|
||||||
|
/// Written into `.git/` so it travels with the checkout, is invisible to the
|
||||||
|
/// repository, and cannot be edited by an agent through its pinned workspace.
|
||||||
|
pub(crate) fn record_base_commit(path: &std::path::Path) {
|
||||||
|
let out = std::process::Command::new("git")
|
||||||
|
.args([
|
||||||
|
"-C",
|
||||||
|
&path.display().to_string(),
|
||||||
|
"-c",
|
||||||
|
&format!("safe.directory={}", path.display()),
|
||||||
|
"rev-parse",
|
||||||
|
"HEAD",
|
||||||
|
])
|
||||||
|
.output();
|
||||||
|
let Ok(out) = out else { return };
|
||||||
|
if !out.status.success() {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
let sha = String::from_utf8_lossy(&out.stdout).trim().to_string();
|
||||||
|
if sha.is_empty() {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
if let Err(e) = std::fs::write(path.join(".git/clawmates-base"), format!("{sha}\n")) {
|
||||||
|
eprintln!(
|
||||||
|
"mission_workspace: could not record base commit for {} ({e}) — delivery will \
|
||||||
|
fall back to diffing against HEAD and will miss committed work",
|
||||||
|
path.display()
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The commit this mission's checkout started from, if it was recorded.
|
||||||
|
pub(crate) fn base_commit(path: &std::path::Path) -> Option<String> {
|
||||||
|
std::fs::read_to_string(path.join(".git/clawmates-base"))
|
||||||
|
.ok()
|
||||||
|
.map(|s| s.trim().to_string())
|
||||||
|
.filter(|s| !s.is_empty())
|
||||||
|
}
|
||||||
|
|
||||||
/// Take the access token back out of `.git/config`.
|
/// Take the access token back out of `.git/config`.
|
||||||
///
|
///
|
||||||
/// `with_ambient_auth` embeds `GITEA_TOKEN` in the clone URL so the clone can
|
/// `with_ambient_auth` embeds `GITEA_TOKEN` in the clone URL so the clone can
|
||||||
@@ -326,12 +375,15 @@ async fn fetch_and_reset(path: &std::path::Path, branch: &str) -> Result<(), Str
|
|||||||
return Err(format!(
|
return Err(format!(
|
||||||
"git reset --hard origin/{branch} → exit {}: {}",
|
"git reset --hard origin/{branch} → exit {}: {}",
|
||||||
reset.status,
|
reset.status,
|
||||||
String::from_utf8_lossy(&reset.stderr)
|
redact_token(&String::from_utf8_lossy(&reset.stderr))
|
||||||
.chars()
|
.chars()
|
||||||
.take(400)
|
.take(400)
|
||||||
.collect::<String>()
|
.collect::<String>()
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
// HEAD just moved to the freshly fetched tip; that is this run's starting
|
||||||
|
// point, so the recorded base moves with it.
|
||||||
|
record_base_commit(path);
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -68,8 +68,19 @@ async fn sweep_once(pool: &PgPool, runtime: &cm_runtime::Runtime) -> Result<(),
|
|||||||
/// all at once.
|
/// all at once.
|
||||||
const CAPTURE_BATCH: i64 = 5;
|
const CAPTURE_BATCH: i64 = 5;
|
||||||
|
|
||||||
/// Write out the diff for coding phases that have finished and not yet been
|
/// Write out the diff for any finished phase of a mission that has a repo.
|
||||||
/// captured.
|
///
|
||||||
|
/// Not just coding phases. `phase_task_text` tells a *research* phase to
|
||||||
|
/// "save findings under /mission/repo/research/ using file_edit", so research
|
||||||
|
/// output is real work sitting in the checkout, and the checkout is deleted
|
||||||
|
/// thirty minutes after the mission ends. Filtering to coding kinds would have
|
||||||
|
/// quietly thrown away every research brief a repo-bearing mission produced.
|
||||||
|
///
|
||||||
|
/// One consequence to know about: `git diff HEAD` is cumulative, so in a
|
||||||
|
/// research→coding mission the coding phase's patch also contains the research
|
||||||
|
/// phase's files. That resolves itself once each phase commits — the next
|
||||||
|
/// phase then diffs against the previous phase's commit rather than the
|
||||||
|
/// original HEAD.
|
||||||
///
|
///
|
||||||
/// Deliberately not hung off `close_finished_phases` or
|
/// Deliberately not hung off `close_finished_phases` or
|
||||||
/// `evaluate_finished_phases`: a phase reaches `completed` through either
|
/// `evaluate_finished_phases`: a phase reaches `completed` through either
|
||||||
@@ -82,7 +93,6 @@ async fn capture_finished_coding_phases(pool: &PgPool) -> Result<(), String> {
|
|||||||
FROM mission_phases mp
|
FROM mission_phases mp
|
||||||
JOIN missions m ON m.id = mp.mission_id
|
JOIN missions m ON m.id = mp.mission_id
|
||||||
WHERE mp.status = 'completed'
|
WHERE mp.status = 'completed'
|
||||||
AND mp.kind IN ('coding', 'benchmark', 'security_scan')
|
|
||||||
AND m.repo_id IS NOT NULL
|
AND m.repo_id IS NOT NULL
|
||||||
AND NOT EXISTS (
|
AND NOT EXISTS (
|
||||||
SELECT 1 FROM mission_artifacts a
|
SELECT 1 FROM mission_artifacts a
|
||||||
|
|||||||
@@ -45,6 +45,21 @@ fn git(repo: &Path, args: &[&str]) {
|
|||||||
|
|
||||||
/// A repo with one commit, at `<root>/<mission>/repo` so `checkout_path`
|
/// A repo with one commit, at `<root>/<mission>/repo` so `checkout_path`
|
||||||
/// finds it.
|
/// finds it.
|
||||||
|
/// Record the clone point the way `mission_workspace` does after a clone.
|
||||||
|
fn record_base(repo: &Path) {
|
||||||
|
let out = Command::new("git")
|
||||||
|
.arg("-C")
|
||||||
|
.arg(repo)
|
||||||
|
.args(["rev-parse", "HEAD"])
|
||||||
|
.output()
|
||||||
|
.unwrap();
|
||||||
|
std::fs::write(
|
||||||
|
repo.join(".git/clawmates-base"),
|
||||||
|
String::from_utf8_lossy(&out.stdout).trim(),
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
}
|
||||||
|
|
||||||
fn seed_repo(root: &Path, mission: Uuid) -> std::path::PathBuf {
|
fn seed_repo(root: &Path, mission: Uuid) -> std::path::PathBuf {
|
||||||
let repo = root.join(mission.to_string()).join("repo");
|
let repo = root.join(mission.to_string()).join("repo");
|
||||||
std::fs::create_dir_all(&repo).unwrap();
|
std::fs::create_dir_all(&repo).unwrap();
|
||||||
@@ -54,6 +69,7 @@ fn seed_repo(root: &Path, mission: Uuid) -> std::path::PathBuf {
|
|||||||
std::fs::write(repo.join("README.md"), "# base\n").unwrap();
|
std::fs::write(repo.join("README.md"), "# base\n").unwrap();
|
||||||
git(&repo, &["add", "."]);
|
git(&repo, &["add", "."]);
|
||||||
git(&repo, &["commit", "--quiet", "-m", "base"]);
|
git(&repo, &["commit", "--quiet", "-m", "base"]);
|
||||||
|
record_base(&repo);
|
||||||
repo
|
repo
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -222,3 +238,77 @@ async fn seed_mission_phase(pool: &sqlx::PgPool, mission: Uuid) -> (Uuid, Uuid)
|
|||||||
.unwrap();
|
.unwrap();
|
||||||
(ws, phase)
|
(ws, phase)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The production failure this pairs with. Mission 019fc372's agent created
|
||||||
|
/// the file it was asked for and *committed* it — `rust_sdlc` has a committer
|
||||||
|
/// role, so that is the intended path — leaving a clean working tree. Capture
|
||||||
|
/// diffed against HEAD, found nothing, and recorded `empty: true` next to a
|
||||||
|
/// commit that plainly contained the work.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn work_the_agent_committed_is_captured() {
|
||||||
|
let pool = cm_testkit::test_pool().await;
|
||||||
|
let tmp = tempfile::tempdir().unwrap();
|
||||||
|
|
||||||
|
let mission = Uuid::now_v7();
|
||||||
|
let repo = seed_repo(tmp.path(), mission);
|
||||||
|
let (_, phase) = seed_mission_phase(&pool, mission).await;
|
||||||
|
|
||||||
|
std::fs::write(repo.join("DELIVERY_PROBE.md"), "CAPTURED-BY-CLAWMATES\n").unwrap();
|
||||||
|
git(&repo, &["add", "DELIVERY_PROBE.md"]);
|
||||||
|
git(&repo, &["commit", "--quiet", "-m", "Add DELIVERY_PROBE.md"]);
|
||||||
|
|
||||||
|
// The tree is clean — `git status --porcelain` is empty here, which is
|
||||||
|
// precisely why the HEAD-relative version saw nothing.
|
||||||
|
let status = Command::new("git")
|
||||||
|
.arg("-C")
|
||||||
|
.arg(&repo)
|
||||||
|
.args(["status", "--porcelain"])
|
||||||
|
.output()
|
||||||
|
.unwrap();
|
||||||
|
assert!(
|
||||||
|
String::from_utf8_lossy(&status.stdout).trim().is_empty(),
|
||||||
|
"the agent committed, so the tree is clean"
|
||||||
|
);
|
||||||
|
|
||||||
|
let cap = capture(&pool, tmp.path(), mission, phase)
|
||||||
|
.await
|
||||||
|
.unwrap()
|
||||||
|
.unwrap();
|
||||||
|
assert!(
|
||||||
|
!cap.empty,
|
||||||
|
"committed work must be captured, not reported as empty"
|
||||||
|
);
|
||||||
|
assert_eq!(cap.files_changed, 1);
|
||||||
|
|
||||||
|
let patch = std::fs::read_to_string(&cap.patch_path).unwrap();
|
||||||
|
assert!(
|
||||||
|
patch.contains("CAPTURED-BY-CLAWMATES"),
|
||||||
|
"the committed content is in the patch"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Committed *and* uncommitted work in the same phase — a coder that committed
|
||||||
|
/// one change and left another in progress.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn committed_and_uncommitted_changes_are_both_captured() {
|
||||||
|
let pool = cm_testkit::test_pool().await;
|
||||||
|
let tmp = tempfile::tempdir().unwrap();
|
||||||
|
|
||||||
|
let mission = Uuid::now_v7();
|
||||||
|
let repo = seed_repo(tmp.path(), mission);
|
||||||
|
let (_, phase) = seed_mission_phase(&pool, mission).await;
|
||||||
|
|
||||||
|
std::fs::write(repo.join("committed.rs"), "fn done() {}\n").unwrap();
|
||||||
|
git(&repo, &["add", "committed.rs"]);
|
||||||
|
git(&repo, &["commit", "--quiet", "-m", "first"]);
|
||||||
|
std::fs::write(repo.join("in_progress.rs"), "fn wip() {}\n").unwrap();
|
||||||
|
|
||||||
|
let cap = capture(&pool, tmp.path(), mission, phase)
|
||||||
|
.await
|
||||||
|
.unwrap()
|
||||||
|
.unwrap();
|
||||||
|
let patch = std::fs::read_to_string(&cap.patch_path).unwrap();
|
||||||
|
assert!(patch.contains("fn done()"), "committed work");
|
||||||
|
assert!(patch.contains("fn wip()"), "uncommitted work");
|
||||||
|
assert_eq!(cap.files_changed, 2);
|
||||||
|
}
|
||||||
|
|||||||
@@ -138,7 +138,14 @@ excluded_tools = ["shell", "file_read", "file_write", "http_request", "browser",
|
|||||||
# burn tokens dumping code inline, this list drifted back to pre-0.8 names.
|
# burn tokens dumping code inline, this list drifted back to pre-0.8 names.
|
||||||
[risk_profiles.coding_readwrite]
|
[risk_profiles.coding_readwrite]
|
||||||
level = "full"
|
level = "full"
|
||||||
allowed_tools = ["file_read", "file_edit", "content_search", "glob_search", "git_operations", "shell"]
|
# `file_write` creates and overwrites; `file_edit` only replaces an exact
|
||||||
|
# existing string and rejects an empty `old_string`, so without file_write an
|
||||||
|
# agent literally cannot create a new file. Observed on mission 019fc372: the
|
||||||
|
# agent burned its turn reasoning about how to make file_edit create a file
|
||||||
|
# ("the tool rejected empty old_string... the shell is restricted") before
|
||||||
|
# working around it through `shell`. The comment below has claimed file_write
|
||||||
|
# was here since the profile was written; the list never had it.
|
||||||
|
allowed_tools = ["file_read", "file_write", "file_edit", "content_search", "glob_search", "git_operations", "shell"]
|
||||||
excluded_tools = ["http_request", "browser", "composio"]
|
excluded_tools = ["http_request", "browser", "composio"]
|
||||||
|
|
||||||
# Read-only research profile (scout/researcher/reviewer/planner roles).
|
# Read-only research profile (scout/researcher/reviewer/planner roles).
|
||||||
|
|||||||
Reference in New Issue
Block a user