fix(missions): unblock the capture batch, and restore fetch auth
Two defects, both found by running a second real coding mission (019fc3ba)
after the first round of fixes. The agent created the file correctly this
time — `file_write` did its job — and capture still produced nothing.
**Head-of-line blocking.** `capture_phase_diff` returns `Ok(None)` when the
checkout is gone, and the caller treated that as success without recording
anything. The phase therefore stayed eligible forever, and because the batch
is bounded at five, five reaped phases from earlier test missions occupied
every slot permanently. A freshly finished coding phase, with its checkout
still on disk, was never reached — and nothing was logged, because nothing had
failed.
Fixed on both axes: an unreachable checkout now writes a `code_diff` marker
recording `captured: false` and why, so the row stops being selected; and the
batch orders newest-first, so live work is captured before archaeology. The
marker also distinguishes "this phase changed nothing" from "we lost the
checkout before looking", which an operator reading the mission needs to be
able to tell apart.
**Fetch lost its credentials.** `scrub_remote_credentials` (P1.1) strips the
token from `.git/config` so agents running as root cannot read it — but
`fetch_and_reset` fetched from the stored remote, which is now anonymous:
git fetch origin <branch> → exit 128:
fatal: could not read Username for 'https://git.redclaw.dev'
I accounted for push building a fresh authenticated URL and overlooked that
fetch needs one too. `fetch_and_reset` now takes the authenticated URL the
caller already computes, as does the `--unshallow` deepen. Stderr stays
redacted.
Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
409ca65ee7
commit
e089360ac8
@@ -317,6 +317,46 @@ pub fn parse_diffstat(stat: &str) -> (usize, usize, usize) {
|
|||||||
(files, ins, del)
|
(files, ins, del)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Mark a phase as impossible to capture, so it stops being selected.
|
||||||
|
///
|
||||||
|
/// A phase whose checkout has already been reaped can never be captured. It
|
||||||
|
/// must still be recorded: the capture batch is bounded, and a row that stays
|
||||||
|
/// eligible forever occupies a slot forever. Enough of them and no live
|
||||||
|
/// mission is ever captured again — head-of-line blocking with a silent
|
||||||
|
/// failure mode, which is how this was found.
|
||||||
|
///
|
||||||
|
/// The artifact is deliberately honest about *why* it is empty. "No changes"
|
||||||
|
/// and "we lost the checkout before looking" are different facts, and an
|
||||||
|
/// operator reading the mission needs to be able to tell them apart.
|
||||||
|
pub async fn record_uncapturable(
|
||||||
|
pool: &sqlx::PgPool,
|
||||||
|
mission_id: Uuid,
|
||||||
|
phase_id: Uuid,
|
||||||
|
) -> Result<(), String> {
|
||||||
|
let rel = format!("_outputs/{mission_id}/{phase_id}/diff.patch");
|
||||||
|
cm_db::repo::missions::register_artifact(
|
||||||
|
pool,
|
||||||
|
cm_db::repo::missions::RegisterArtifact {
|
||||||
|
mission_id,
|
||||||
|
phase_id: Some(phase_id),
|
||||||
|
path: &rel,
|
||||||
|
kind: "code_diff",
|
||||||
|
mime: Some("text/x-patch"),
|
||||||
|
title: Some("Not captured — checkout unavailable"),
|
||||||
|
generated_by_run: None,
|
||||||
|
render_pdf: false,
|
||||||
|
metadata: Some(json!({
|
||||||
|
"empty": true,
|
||||||
|
"captured": false,
|
||||||
|
"reason": "the mission checkout was removed before the diff could be captured",
|
||||||
|
})),
|
||||||
|
},
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
.map(|_| ())
|
||||||
|
.map_err(|e| format!("register uncapturable marker: {e}"))
|
||||||
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
|
|||||||
@@ -67,7 +67,7 @@ pub async fn ensure_checkout(
|
|||||||
|
|
||||||
let auth_url = with_ambient_auth(clone_url);
|
let auth_url = with_ambient_auth(clone_url);
|
||||||
if path.join(".git").exists() {
|
if path.join(".git").exists() {
|
||||||
fetch_and_reset(&path, default_branch).await?;
|
fetch_and_reset(&path, default_branch, &auth_url).await?;
|
||||||
} else {
|
} else {
|
||||||
clone(&path, &auth_url).await?;
|
clone(&path, &auth_url).await?;
|
||||||
}
|
}
|
||||||
@@ -311,7 +311,11 @@ fn redact_token(s: &str) -> String {
|
|||||||
out
|
out
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn fetch_and_reset(path: &std::path::Path, branch: &str) -> Result<(), String> {
|
async fn fetch_and_reset(
|
||||||
|
path: &std::path::Path,
|
||||||
|
branch: &str,
|
||||||
|
auth_url: &str,
|
||||||
|
) -> Result<(), String> {
|
||||||
// A checkout cloned before delivery existed is shallow, and a shallow repo
|
// A checkout cloned before delivery existed is shallow, and a shallow repo
|
||||||
// cannot push a new branch. Deepen it once, here, rather than discovering
|
// cannot push a new branch. Deepen it once, here, rather than discovering
|
||||||
// the problem at push time when there is work on the line. `--unshallow`
|
// the problem at push time when there is work on the line. `--unshallow`
|
||||||
@@ -324,7 +328,7 @@ async fn fetch_and_reset(path: &std::path::Path, branch: &str) -> Result<(), Str
|
|||||||
&path.display().to_string(),
|
&path.display().to_string(),
|
||||||
"fetch",
|
"fetch",
|
||||||
"--unshallow",
|
"--unshallow",
|
||||||
"origin",
|
auth_url,
|
||||||
])
|
])
|
||||||
.output()
|
.output()
|
||||||
.await;
|
.await;
|
||||||
@@ -345,8 +349,14 @@ async fn fetch_and_reset(path: &std::path::Path, branch: &str) -> Result<(), Str
|
|||||||
),
|
),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// Fetch from an explicitly authenticated URL rather than the stored
|
||||||
|
// remote. `scrub_remote_credentials` strips the token out of
|
||||||
|
// `.git/config` — the checkout is readable by agents running as root —
|
||||||
|
// so `git fetch origin` has no credentials and fails with
|
||||||
|
// "could not read Username". Building the URL here also means a rotated
|
||||||
|
// token takes effect immediately instead of at the next clone.
|
||||||
let fetch = Command::new("git")
|
let fetch = Command::new("git")
|
||||||
.args(["-C", &path.display().to_string(), "fetch", "origin", branch])
|
.args(["-C", &path.display().to_string(), "fetch", auth_url, branch])
|
||||||
.output()
|
.output()
|
||||||
.await
|
.await
|
||||||
.map_err(|e| format!("spawn git fetch: {e}"))?;
|
.map_err(|e| format!("spawn git fetch: {e}"))?;
|
||||||
|
|||||||
@@ -100,7 +100,7 @@ async fn capture_finished_coding_phases(pool: &PgPool) -> Result<(), String> {
|
|||||||
AND a.phase_id = mp.id
|
AND a.phase_id = mp.id
|
||||||
AND a.kind = 'code_diff'
|
AND a.kind = 'code_diff'
|
||||||
)
|
)
|
||||||
ORDER BY mp.completed_at NULLS LAST
|
ORDER BY mp.completed_at DESC NULLS LAST
|
||||||
LIMIT $1",
|
LIMIT $1",
|
||||||
)
|
)
|
||||||
.bind(CAPTURE_BATCH)
|
.bind(CAPTURE_BATCH)
|
||||||
@@ -112,15 +112,29 @@ async fn capture_finished_coding_phases(pool: &PgPool) -> Result<(), String> {
|
|||||||
use sqlx::Row;
|
use sqlx::Row;
|
||||||
let phase_id: Uuid = row.get("id");
|
let phase_id: Uuid = row.get("id");
|
||||||
let mission_id: Uuid = row.get("mission_id");
|
let mission_id: Uuid = row.get("mission_id");
|
||||||
if let Err(e) =
|
match crate::mission_delivery::capture_phase_diff(pool, mission_id, phase_id).await {
|
||||||
crate::mission_delivery::capture_phase_diff(pool, mission_id, phase_id).await
|
Ok(Some(_)) => {}
|
||||||
{
|
Ok(None) => {
|
||||||
// Left uncaptured on purpose: the guard above re-selects it next
|
// The checkout is gone — reaped before capture reached this
|
||||||
// tick. Only a permanently broken checkout keeps failing, and that
|
// phase. Record that, or the row stays eligible forever; and
|
||||||
// is worth the recurring log line.
|
// because the batch is bounded, a handful of dead phases
|
||||||
eprintln!(
|
// occupy every slot permanently and no live mission is ever
|
||||||
"phase_runner: capturing diff for mission {mission_id} phase {phase_id}: {e}"
|
// captured again. That is exactly how this was found: five
|
||||||
);
|
// reaped phases from earlier runs blocked the batch while a
|
||||||
|
// freshly finished coding phase went untouched.
|
||||||
|
if let Err(e) =
|
||||||
|
crate::mission_delivery::record_uncapturable(pool, mission_id, phase_id).await
|
||||||
|
{
|
||||||
|
eprintln!("phase_runner: recording uncapturable phase {phase_id}: {e}");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
Err(e) => {
|
||||||
|
// A real failure against a checkout that still exists; the
|
||||||
|
// next tick retries it.
|
||||||
|
eprintln!(
|
||||||
|
"phase_runner: capturing diff for mission {mission_id} phase {phase_id}: {e}"
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
Ok(())
|
Ok(())
|
||||||
|
|||||||
Reference in New Issue
Block a user