fix(repo): write-tier reliability — commit gpg-hang, push 300s hang, silent push failure (#815)

src/harness/repo.rs

@@ -573,18 +573,30 @@ fn file_url(path: &Path) -> String {
     format!("file://{}", path.display())
 }
 
-/// Sets a fresh checkout's commit identity to the agent's seat (issue #735).
+/// Sets a fresh checkout's commit identity to the agent's seat (issue #735) and
+/// turns commit signing off (issue #796).
 ///
 /// git makes commits with the repository's own `user.name`/`user.email`, so
 /// setting them here — before the agent can commit through `git_operations` —
 /// is what attributes the branch it may later publish to the agent rather than
 /// to a shared machine identity. The address is synthetic and non-routable; it
 /// exists to identify, not to receive mail. Best-effort: a config that will not
 /// set is logged, and a checkout that is only ever read is unaffected either way.
+///
+/// `commit.gpgsign` / `tag.gpgsign` are forced to `false` because the clone
+/// inherits the host operator's global git config: on a host that signs its own
+/// commits (`commit.gpgsign = true`), every agent `git commit` would block on a
+/// GPG key the sandbox has no way to reach — the commit hangs or fails and the
+/// whole write flow stalls with the change staged but never committed. The
+/// agent's commits are attributed by identity, not signed by a key it does not
+/// hold; per-agent commit signing is the deferred issue #738.
 async fn attribute_checkout(dest: &Path, agent: &str) {
     for (key, value) in [
         ("user.name", agent.to_string()),
         ("user.email", format!("{agent}@agents.opencompany.local")),
+        // Issue #796: never inherit the host's `commit.gpgsign = true`.
+        ("commit.gpgsign", "false".to_string()),
+        ("tag.gpgsign", "false".to_string()),
     ] {
         match git::run(dest, &["config", key, &value], None, None).await {
             Ok(out) if out.ok => {}

src/harness/repo/test.rs

@@ -1166,6 +1166,30 @@ async fn committed_checkout(ctx: &RepoToolContext, mirror: &Path, key: &str) {
     git_at(&dest, &["commit", "--quiet", "-m", "the fix"]);
 }
 
+/// Issue #796: a materialized checkout has commit/tag signing turned OFF, so an
+/// agent's `git commit` never blocks on a GPG key the sandbox cannot reach —
+/// even when the host operator's own git config turns signing on. Without this
+/// the change stages but never commits and the whole write flow stalls.
+#[tokio::test]
+async fn a_checkout_disables_commit_signing() {
+    let scratch = Scratch::new("no-gpgsign");
+    let dest = scratch.join("checkout");
+    std::fs::create_dir_all(&dest).unwrap();
+    git_at(&dest, &["init", "--quiet"]);
+    // A host that signs its own commits.
+    git_at(&dest, &["config", "commit.gpgsign", "true"]);
+
+    attribute_checkout(&dest, "coder").await;
+
+    assert_eq!(
+        git_at(&dest, &["config", "--get", "commit.gpgsign"]),
+        "false"
+    );
+    assert_eq!(git_at(&dest, &["config", "--get", "tag.gpgsign"]), "false");
+    // And the identity is still the agent's seat (issue #735).
+    assert_eq!(git_at(&dest, &["config", "--get", "user.name"]), "coder");
+}
+
 /// Publishing outside a task refuses — this tier is task turns only, and there
 /// is no card to name the branch. Nothing is staged and nothing is queued.
 #[tokio::test]

src/runtime/cycle.rs

@@ -1307,6 +1307,8 @@ async fn perform_effect(rt: &CompanyRuntime, effect: &Effect) -> Result<()> {
             .get("head")
             .and_then(|v| v.as_str())
             .unwrap_or_default();
+        // The card this publish belongs to, so a host-side failure can be reported
+        // on it.
         let task = effect
             .payload
             .get("task")
@@ -1328,8 +1330,36 @@ async fn perform_effect(rt: &CompanyRuntime, effect: &Effect) -> Result<()> {
                  configured to perform it",
             ));
         };
-        // The push is the irreversible half: its failure fails the effect.
-        repos.push_published(repo, branch, head).await?;
+        // The push is the irreversible half, and it fails the effect — but never
+        // silently (issue #815). A failed push leaves the effect recorded as
+        // executed (the at-most-once guard), so re-approving is a no-op; the
+        // operator has to KNOW it failed and re-run the task, or the publish
+        // vanishes with the change staged in the mirror and nothing on the remote.
+        if let Err(err) = repos.push_published(repo, branch, head).await {
+            tracing::warn!(branch, "[repo] could not publish the branch: {err}");
+            if !task.is_empty() {
+                let note = format!(
+                    "Could not publish `{branch}` to the remote: {err}. Nothing reached the \
+                     remote, and this approval will not retry on its own \u{2014} re-run the task \
+                     to publish again."
+                );
+                if let Err(e) = rt
+                    .events
+                    .append(
+                        &rt.id,
+                        CompanyEvent::TaskDiscussionPosted {
+                            task_id: task.to_string(),
+                            text: note,
+                            by: None,
+                        },
+                    )
+                    .await
+                {
+                    tracing::warn!("[repo] could not record the push failure on the task: {e}");
+                }
+            }
+            return Err(err);
+        }
 
         // Issue #736: open a pull request for the pushed branch, best-effort. The
         // push has landed, so a PR failure must NOT fail the effect — the branch
@@ -1352,8 +1382,8 @@ async fn perform_effect(rt: &CompanyRuntime, effect: &Effect) -> Result<()> {
                 if !task.is_empty() {
                     let note = format!(
                         "Published `{branch}` to the remote, but the pull request could not be \
-                         opened: {err}. The branch is on the remote — open a PR from it by hand, \
-                         or approve another publish to retry."
+                         opened: {err}. The branch is on the remote \u{2014} open a PR from it by \
+                         hand, or approve another publish to retry."
                     );
                     if let Err(e) = rt
                         .events
@@ -1497,7 +1527,35 @@ fn sanitize_work_segment(thread: &str) -> Option<String> {
     // Bound the body well under `validate_task_segment`'s 128-char cap; the
     // characters are all ASCII, so a byte take is a char take.
     let body: String = cleaned.chars().take(100).collect();
-    Some(format!("dm-{body}"))
+    // Injective, not just safe. Folding every disallowed character to `-` — which
+    // is itself a keep-character — and trimming/truncating are all lossy, so two
+    // distinct threads can reduce to one body: `coder/main` and `coder-main` both
+    // become `coder-main`. Since this value keys checkout retention and the
+    // `oc/<company>/<unit>` publish branch, a collision would let one thread
+    // reclaim another's tree or publish over its branch. When anything was lost,
+    // append a short stable digest of the *raw* thread so distinct threads keep
+    // distinct keys; a thread that was already a safe segment is unchanged, so
+    // its key stays readable.
+    if body == thread {
+        Some(format!("dm-{body}"))
+    } else {
+        Some(format!("dm-{body}-{}", short_thread_digest(thread)))
+    }
+}
+
+/// A short, build-stable digest of a raw thread id (64-bit FNV-1a), used to keep
+/// two threads that sanitise to the same body from sharing a work key.
+///
+/// A `std` `DefaultHasher` is deliberately not used: its output is not
+/// guaranteed stable across toolchain versions, and this digest names a durable
+/// branch and checkout key that must hash the same on every build.
+fn short_thread_digest(thread: &str) -> String {
+    let mut hash: u64 = 0xcbf2_9ce4_8422_2325;
+    for byte in thread.bytes() {
+        hash ^= byte as u64;
+        hash = hash.wrapping_mul(0x0000_0100_0000_01b3);
+    }
+    format!("{hash:016x}")
 }
 
 /// The board task a cycle is working, read off its own trigger events
@@ -2345,19 +2403,34 @@ mod test {
     /// broken ref.
     #[test]
     fn sanitize_work_segment_makes_a_safe_branch_segment() {
+        // An already-safe thread is unchanged and keeps a readable key.
         assert_eq!(sanitize_work_segment("coder"), Some("dm-coder".into()));
-        // Colons, slashes and spaces fold to '-'.
-        assert_eq!(
-            sanitize_work_segment("dm:coder/main x"),
-            Some("dm-dm-coder-main-x".into())
-        );
-        // Leading/trailing separators are trimmed before the prefix.
-        assert_eq!(sanitize_work_segment("--weird--"), Some("dm-weird".into()));
         // Dots and underscores are already valid and survive.
         assert_eq!(sanitize_work_segment("a_b.c"), Some("dm-a_b.c".into()));
         // Nothing usable.
         assert_eq!(sanitize_work_segment(""), None);
         assert_eq!(sanitize_work_segment("///"), None);
+
+        // When folding/trimming loses information, the readable body is kept and
+        // a digest of the raw thread is appended so distinct threads never share
+        // a work key. Colons, slashes and spaces fold to '-'; leading/trailing
+        // separators are trimmed before the prefix.
+        let folded = sanitize_work_segment("dm:coder/main x").unwrap();
+        assert!(folded.starts_with("dm-dm-coder-main-x-"), "{folded}");
+        let trimmed = sanitize_work_segment("--weird--").unwrap();
+        assert!(trimmed.starts_with("dm-weird-"), "{trimmed}");
+
+        // The collision the digest closes: two threads that fold to the same body
+        // get distinct keys — and the digest is deterministic across calls.
+        assert_ne!(
+            sanitize_work_segment("coder/main"),
+            sanitize_work_segment("coder-main"),
+            "distinct threads must not share a work key"
+        );
+        assert_eq!(
+            sanitize_work_segment("coder/main"),
+            sanitize_work_segment("coder/main")
+        );
     }
     use std::sync::Arc;
     use std::sync::atomic::AtomicUsize;