fix(delivery): four failures from the #55 trace — auth, prompts, truncation, retry
All four were surfaced while tracing #55 and left open. Each one on its own is
small; together they are why a two-line git rejection took hours to read.
**1. `with_ambient_auth` failed open.** It matched one literal prefix,
`https://git.redclaw.dev/`, and returned the URL unchanged for everything else
with no log line. An `http://` remote, an explicit port, a different case in the
host, an ssh remote, a URL that already carried userinfo — all came back
unauthenticated and looked identical to success. It now returns `Authed`, which
carries the URL AND why no credential reached it, and recognises the forge in
every shape a remote can be written (host parsed with userinfo stripped BEFORE
the port, or `oauth2:token@host` reports its username as the host — the first
version of this function did exactly that and failed its own test).
**2. Nothing set `GIT_TERMINAL_PROMPT=0`.** So a credential-less URL did not
fail — git opened `/dev/tty`, and in a server container that surfaces as
`No such device or address`, several layers from the missing token. Now set on
every git invocation that can reach the network. And `push_url_for` refuses
outright when the URL is on OUR forge and unauthenticated: that push cannot
succeed, and letting it proceed only buys a symptom that looks like something
else.
**3. The truncation fix went to the wrong path.** e31688b clamped the caller,
but a rejected push comes back as `Ok(Publish { error })` — the string was
already cut to 300 head chars inside `git()`, so the reject reason had been
dropped before the both-ends clamp ever saw it. Clamped where the output is
produced, and redacted there too.
**4. #55: a mission that re-clones can never push.** The branch name is
deterministic per (mission, phase, iteration), so a checkout rebuilt after a
retry, a container teardown or disk loss produces divergent history against its
own branch, and git rejects it — leaving the work on a local branch in a
directory the sweeper deletes. Reachable in normal operation, not just by
deleting a checkout by hand.
The escape is a NEW ref, not `--force`: forcing would overwrite whatever the
earlier attempt pushed, which may be the only copy of that work, to make this
attempt look tidy. The retry lands on `<branch>-<sha8>` — deterministic,
self-describing in a branch list, and collision-free since divergent history is
by definition a different sha. "Never force" stays a rule.
NEGATIVE CONTROL, run rather than assumed: with the rescue arm disabled,
`diverged_history_lands_on_a_new_branch_instead_of_being_lost` FAILS with git's
real `! [rejected] ... (fetch first)` — which also demonstrates fix 3, since that
whole message now survives to the assertion. The test asserts the earlier
attempt's ref is byte-identical afterwards.
Also measured, not read off docs: which hooks fire under `claude -p` (2.1.222,
via `--settings`). SessionStart, UserPromptSubmit, PreToolUse, PostToolUse,
SubagentStop and Stop fire; TaskCreated, TaskCompleted, TeammateIdle, SessionEnd,
Notification and PreCompact do not. So the agent-teams hooks Slice 3 deferred are
inert on our path by construction, and `Stop` is the seam that could move
`done_when` into the agent's own loop.
507 tests pass, clippy clean.
Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
Omar Sobh
co-authored by
Claude Opus 5
Claude Opus 5
parent
12147a1e01
commit
1d554396f4
@@ -511,8 +511,9 @@ async fn git(repo: &Path, args: &[&str]) -> Result<String, String> {
|
||||
];
|
||||
full.extend(args.iter().map(|a| (*a).to_string()));
|
||||
let (name, email) = commit_identity();
|
||||
let out = tokio::process::Command::new("git")
|
||||
.args(&full)
|
||||
let mut cmd = tokio::process::Command::new("git");
|
||||
cmd.args(&full);
|
||||
let out = crate::mission_workspace::no_terminal_prompt(&mut cmd)
|
||||
// The server container has no git identity — `git config --global
|
||||
// user.email` exits 1 — so `git commit` fails with "Author identity
|
||||
// unknown" unless one is supplied. Mission `019fc450` lost its first
|
||||
@@ -541,10 +542,15 @@ async fn git(repo: &Path, args: &[&str]) -> Result<String, String> {
|
||||
"git {} → {}: {}",
|
||||
args.first().copied().unwrap_or("?"),
|
||||
out.status,
|
||||
String::from_utf8_lossy(&out.stderr)
|
||||
.chars()
|
||||
.take(300)
|
||||
.collect::<String>()
|
||||
// Both ends, never a head-only clamp. THIS is where the reason was
|
||||
// being lost: `publish_phase_branch` returns a rejected push as
|
||||
// `Ok(Publish { error })`, so the string it carries was already
|
||||
// truncated here — 300 chars of auth noise, with "non-fast-forward"
|
||||
// cut off — before the caller's own both-ends clamp ever saw it.
|
||||
// Clamping the caller fixed the path that was already fine.
|
||||
crate::mission_workspace::redact_token(&crate::evaluator_tools::clamp_output(
|
||||
&String::from_utf8_lossy(&out.stderr)
|
||||
))
|
||||
));
|
||||
}
|
||||
Ok(String::from_utf8_lossy(&out.stdout).into_owned())
|
||||
@@ -779,7 +785,38 @@ async fn push_url_for(pool: &sqlx::PgPool, mission_id: Uuid) -> Result<Option<St
|
||||
// "nothing to push to" — the shape that made `commit_error` necessary.
|
||||
.map_err(|e| format!("query push URL: {e}"))?
|
||||
.flatten();
|
||||
Ok(url.map(|u| mission_workspace::with_ambient_auth(&u)))
|
||||
let Some(url) = url else { return Ok(None) };
|
||||
|
||||
let auth = mission_workspace::with_ambient_auth(&url);
|
||||
// Fail here, not at the tty. An unauthenticated URL to OUR forge cannot
|
||||
// push, and every second it survives past this point is spent producing a
|
||||
// symptom that looks like something else: git asking for a username, then
|
||||
// `/dev/tty: No such device or address`, then a `push_error` about auth that
|
||||
// sent #55's investigation after credentials which were never the problem.
|
||||
// A third-party host is left alone — ssh keys and .netrc are legitimate.
|
||||
if let Some(why) = &auth.unauthenticated {
|
||||
if auth.is_forge() {
|
||||
return Err(format!(
|
||||
"cannot authenticate the push URL for this mission — {why}. The work \
|
||||
is committed locally; fix the credential and re-run delivery."
|
||||
));
|
||||
}
|
||||
eprintln!("mission_delivery: pushing to a non-forge remote unauthenticated — {why}");
|
||||
}
|
||||
Ok(Some(auth.url))
|
||||
}
|
||||
|
||||
/// Did the forge reject this push because our history diverged from the ref?
|
||||
///
|
||||
/// Git says this several ways depending on version and refspec, and all of them
|
||||
/// mean the same thing here: the branch already exists with commits ours does not
|
||||
/// contain.
|
||||
fn is_non_fast_forward(err: &str) -> bool {
|
||||
let e = err.to_ascii_lowercase();
|
||||
e.contains("non-fast-forward")
|
||||
|| e.contains("fetch first")
|
||||
|| e.contains("updates were rejected")
|
||||
|| (e.contains("[rejected]") && !e.contains("stale info"))
|
||||
}
|
||||
|
||||
/// Run the gate, then push the branch if the gate allows it.
|
||||
@@ -822,6 +859,54 @@ pub async fn publish_phase_branch(
|
||||
error: None,
|
||||
})
|
||||
}
|
||||
// #55: a mission whose checkout was re-cloned — a retry, a container
|
||||
// teardown, disk loss — builds divergent history against its OWN
|
||||
// deterministic branch, and every push it ever attempts is rejected.
|
||||
// Before this, that was terminal: the work stayed on a local branch in a
|
||||
// directory that gets reaped.
|
||||
//
|
||||
// The escape is a NEW ref, not `--force`. Forcing would overwrite
|
||||
// whatever the earlier attempt pushed — which may be the only copy of
|
||||
// that work — to make this attempt look tidy. Suffixing with the commit
|
||||
// sha is deterministic (the same history always lands on the same ref),
|
||||
// self-describing in a branch list, and cannot collide, since divergent
|
||||
// history is by definition a different sha.
|
||||
Err(e) if is_non_fast_forward(&e) => {
|
||||
let sha = git(repo, &["rev-parse", "HEAD"])
|
||||
.await
|
||||
.map(|s| s.trim().to_string())
|
||||
.unwrap_or_default();
|
||||
let Some(short) = sha.get(..8) else {
|
||||
eprintln!("mission_delivery: push of {target} rejected and HEAD unreadable: {e}");
|
||||
return Ok(Publish {
|
||||
branch: target,
|
||||
pushed: false,
|
||||
error: Some(e),
|
||||
});
|
||||
};
|
||||
let alt = format!("{target}-{short}");
|
||||
eprintln!(
|
||||
"mission_delivery: {target} exists on the forge with history this \
|
||||
checkout does not contain — pushing to {alt} instead of forcing. \
|
||||
Original: {e}"
|
||||
);
|
||||
git(repo, &["branch", "-f", &alt, "HEAD"]).await?;
|
||||
match git(repo, &["push", push_url, &format!("HEAD:refs/heads/{alt}")]).await {
|
||||
Ok(_) => Ok(Publish {
|
||||
branch: alt,
|
||||
pushed: true,
|
||||
// Not an error — the work reached the forge — but the
|
||||
// redirect is a fact the operator needs, or two branches for
|
||||
// one phase look like a bug rather than a rescue.
|
||||
error: None,
|
||||
}),
|
||||
Err(e2) => Ok(Publish {
|
||||
branch: alt,
|
||||
pushed: false,
|
||||
error: Some(format!("{e}\n\nand the diverged-history retry also failed: {e2}")),
|
||||
}),
|
||||
}
|
||||
}
|
||||
Err(e) => {
|
||||
// Redacted by `git`'s error path already; the patch and the local
|
||||
// branch both survive, so this is a degraded success.
|
||||
@@ -1041,6 +1126,38 @@ fn untrusted_empty_reason(empty: bool, diff_error: Option<&str>) -> Option<Strin
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// Git says "your history diverged" several ways, and the one production
|
||||
/// actually produced (`! [rejected] ... (fetch first)`) is not the phrase
|
||||
/// anyone reaches for first. Missing a phrasing means the rescue does not
|
||||
/// fire and the work stays on a local branch in a directory that gets
|
||||
/// reaped — silently, since the push failure is a degraded success.
|
||||
#[test]
|
||||
fn every_way_git_says_diverged_is_recognised() {
|
||||
for e in [
|
||||
"git push → exit 1: ! [rejected] HEAD -> b (fetch first)\nhint: …",
|
||||
" ! [rejected] HEAD -> b (non-fast-forward)",
|
||||
"hint: Updates were rejected because the remote contains work that you \
|
||||
do not have locally.",
|
||||
] {
|
||||
assert!(is_non_fast_forward(e), "not recognised: {e}");
|
||||
}
|
||||
}
|
||||
|
||||
/// And it must not fire on failures a new branch cannot fix. Retrying a
|
||||
/// permissions or network error onto a second ref just produces a second
|
||||
/// failure and a confusing branch name.
|
||||
#[test]
|
||||
fn other_push_failures_are_not_mistaken_for_divergence() {
|
||||
for e in [
|
||||
"fatal: repository 'https://forge/x.git' not found",
|
||||
"remote: error: GH006: Protected branch update failed",
|
||||
"fatal: could not read Username for 'https://forge': terminal prompts disabled",
|
||||
" ! [rejected] (stale info)",
|
||||
] {
|
||||
assert!(!is_non_fast_forward(e), "wrongly recognised: {e}");
|
||||
}
|
||||
}
|
||||
|
||||
// ── The gate ───────────────────────────────────────────────────────
|
||||
|
||||
/// Three recipes have declared `commit_policy` since they were written and
|
||||
|
||||
Reference in New Issue
Block a user