Sync the commit a webhook delivered, not the branch

Found by watching the real thing: a push to the handbook repository returned
200, and the page did not change. A manual sync a minute later applied exactly
one page. The webhook had synced the tree from before the push that triggered it.

A host serves the branch archive from a cache, and a webhook arrives the instant
the push lands, so `archive/main.zip` can still be the previous tree. The
workspace then sits one commit behind and looks perfectly healthy, because every
push applies the previous one — which is why nothing caught it until a push was
watched end to end.

The delivery names the commit, so that is what gets read. Immune to the cache,
and more honest anyway: it applies the tree that was pushed rather than whatever
the branch points at by the time the download runs. The commit also becomes the
ref in the activity log, which used to just repeat the branch name.

The delivery also names the branch, so a push to one this workspace does not
follow is now ignored rather than triggering a sync of the configured branch.
Reported as OK, not FAILED: that delivery worked exactly as intended, and a red
status would be a repository with nothing wrong with it.

Uncovered a second defect while testing the first. A webhook sync applied its
documents and recorded none of the outcome — no status, no message, no commit,
no problems — because the controller passed an entity that findForWebhook had
loaded in its own read-only transaction, and mutations to a detached entity go
nowhere. The manual path was unaffected, since it loads inside its own
transaction, so the settings screen was right after Sync now and said "nothing
yet" after every webhook. syncFromWebhook now takes the id and loads it itself.
Probed by handing it a detached instance again.

506 backend tests.

Author: Mario Ferreira

backend/src/main/java/com/devforge/sync/api/SyncController.java

@@ -160,7 +160,7 @@ public ResponseEntity<Map<String, Object>> webhook(
             return ResponseEntity.notFound().build();
         }
 
-        SyncOutcome outcome = syncService.syncFromWebhook(webhook.configuration());
+        SyncOutcome outcome = syncService.syncFromWebhook(webhookId, payload);
 
         // 200 even for a failed sync: the delivery was accepted and understood, and a
         // git host retrying because DevForge could not reach the repository would not

backend/src/main/java/com/devforge/sync/application/DocumentSource.java

@@ -17,8 +17,12 @@ public interface DocumentSource {
 
     /**
      * @param accessToken decrypted, or null for a public repository
+     * @param revision    the exact revision to read, or null for the configured
+     *                    branch. A webhook names the commit it just delivered,
+     *                    which a branch name cannot be relied on to resolve to —
+     *                    see {@link PushEvent}.
      * @throws SourceUnavailableException when the repository cannot be read; the
      *         message is shown to the operator, so it must say something useful
      */
-    SourceSnapshot fetch(SyncConfiguration configuration, String accessToken);
+    SourceSnapshot fetch(SyncConfiguration configuration, String accessToken, String revision);
 }

backend/src/main/java/com/devforge/sync/application/PushEvent.java

@@ -0,0 +1,92 @@
+package com.devforge.sync.application;
+
+import tools.jackson.core.JacksonException;
+import tools.jackson.databind.JsonNode;
+import tools.jackson.databind.json.JsonMapper;
+
+import java.nio.charset.StandardCharsets;
+import java.util.Optional;
+
+/**
+ * What a git host said it just pushed.
+ *
+ * <p>Read for one reason: <strong>the commit</strong>. Syncing the branch name
+ * instead looks equivalent and is not — a host serves {@code archive/main.zip}
+ * from a cache, and a webhook arrives at the instant the push lands, so the
+ * download can return the tree from <em>before</em> the push that triggered it.
+ * The workspace then sits one commit behind and looks perfectly healthy, because
+ * every push applies the previous one.
+ *
+ * <p>Naming the commit is immune to that, and is the more honest thing to do
+ * anyway: it applies the tree that was actually pushed rather than whatever the
+ * branch happens to point at by the time the download runs.
+ *
+ * <p>The branch matters too. Without it, a push to any branch syncs the configured
+ * one, so a draft branch triggers work that has nothing to do with it.
+ *
+ * <p>GitHub, GitLab, Gitea and Forgejo all send {@code ref} and {@code after} at
+ * the top level, so one shape covers every host DevForge claims to support.
+ *
+ * @param ref      the full ref that moved, such as {@code refs/heads/main}
+ * @param commitId the ref's new tip
+ */
+public record PushEvent(String ref, String commitId) {
+
+    private static final JsonMapper MAPPER = JsonMapper.builder().build();
+
+    /** All zeros is how a host says a branch was deleted; there is no tree to fetch. */
+    private static final String DELETED = "0000000000000000000000000000000000000000";
+
+    /**
+     * @return empty for a body that is not a push — a ping, a pull request, a
+     *         branch deletion, or anything unparseable. The caller falls back to
+     *         syncing the configured branch, which is what happened before any of
+     *         this was read and is never worse than refusing.
+     */
+    public static Optional<PushEvent> parse(byte[] payload) {
+        if (payload == null || payload.length == 0) {
+            return Optional.empty();
+        }
+
+        JsonNode root;
+        try {
+            root = MAPPER.readTree(new String(payload, StandardCharsets.UTF_8));
+        } catch (JacksonException notJson) {
+            return Optional.empty();
+        }
+
+        if (root == null || !root.isObject()) {
+            return Optional.empty();
+        }
+
+        String ref = text(root, "ref");
+        String after = text(root, "after");
+        // GitLab sends `checkout_sha` alongside `after`; they agree on a push, and
+        // this is only a fallback for a host that omits one of them.
+        String commit = after != null ? after : text(root, "checkout_sha");
+
+        if (ref == null || commit == null || commit.equals(DELETED)) {
+            return Optional.empty();
+        }
+        return Optional.of(new PushEvent(ref, commit));
+    }
+
+    /** Whether this push moved the branch a workspace follows. */
+    public boolean movedBranch(String branch) {
+        return branch != null
+                && (ref.equals("refs/heads/" + branch) || ref.equals(branch));
+    }
+
+    /** The branch's short name, for saying which one was ignored. */
+    public String branchName() {
+        return ref.startsWith("refs/heads/") ? ref.substring("refs/heads/".length()) : ref;
+    }
+
+    private static String text(JsonNode root, String field) {
+        JsonNode value = root.get(field);
+        if (value == null || !value.isTextual() || value.asString().isBlank()) {
+            return null;
+        }
+        return value.asString();
+    }
+}

backend/src/main/java/com/devforge/sync/application/SyncOutcome.java

@@ -25,4 +25,16 @@ public record SyncOutcome(
     public static SyncOutcome failed(String message) {
         return new SyncOutcome(SyncStatus.FAILED, null, 0, 0, 0, 0, List.of(), message);
     }
+
+    /**
+     * Nothing was attempted, and nothing was wrong.
+     *
+     * <p>Reported as OK rather than FAILED: a push to a branch this workspace does
+     * not follow is a delivery working exactly as intended, and recording it as a
+     * failure would leave the settings screen showing a red status for a repository
+     * with nothing wrong with it.
+     */
+    public static SyncOutcome ignored(String message) {
+        return new SyncOutcome(SyncStatus.OK, null, 0, 0, 0, 0, List.of(), message);
+    }
 }

backend/src/main/java/com/devforge/sync/application/SyncRunner.java

@@ -70,6 +70,16 @@ public SyncRunner(
      */
     @Transactional
     public SyncOutcome run(SyncConfiguration configuration, UUID actorId) {
+        return run(configuration, actorId, null);
+    }
+
+    /**
+     * @param revision the exact revision to apply, or null for the configured
+     *                 branch. A webhook names the commit it delivered, which is
+     *                 both more accurate and immune to a host serving a cached
+     *                 archive of the branch — see {@link PushEvent}.
+     */
+    public SyncOutcome run(SyncConfiguration configuration, UUID actorId, String revision) {
         configuration.recordAttempt();
 
         if (!configuration.isConfigured()) {
@@ -89,7 +99,7 @@ public SyncOutcome run(SyncConfiguration configuration, UUID actorId) {
                         + "DEVFORGE_JWT_SECRET was changed. Enter it again to reconnect."),
                         actorId);
             }
-            snapshot = source.fetch(configuration, token);
+            snapshot = source.fetch(configuration, token, revision);
         } catch (SourceUnavailableException e) {
             log.warn("Sync fetch failed for workspace {}: {}",
                     configuration.getWorkspaceId(), e.getMessage());

backend/src/main/java/com/devforge/sync/application/SyncService.java

@@ -205,13 +205,38 @@ public SyncSettingsResponse syncNow(UUID workspaceId, UUID userId) {
     /**
      * Runs a sync in response to a verified webhook.
      *
+     * <p>Takes the id and loads the configuration again rather than accepting the
+     * one {@link #findForWebhook} handed the controller. That instance came from a
+     * different transaction and is detached, so everything a run records on it — the
+     * status, the message, the commit, the problems — would be written to an object
+     * JPA is no longer tracking and silently dropped. The documents were still
+     * applied, which is what made it hard to notice: the sync worked, and the
+     * settings screen said it had never run.
+     *
      * <p>No workspace permission is checked, and none could be: a git host has no
      * DevForge account. The signature is what authorises the call, which is why
      * {@link #findForWebhook} refuses to hand back a configuration without one.
      */
     @Transactional
-    public SyncOutcome syncFromWebhook(SyncConfiguration configuration) {
-        return runner.run(configuration, null);
+    public SyncOutcome syncFromWebhook(UUID webhookId, byte[] payload) {
+        SyncConfiguration configuration = repository.findByWebhookId(webhookId)
+                .filter(SyncConfiguration::isEnabled)
+                .orElseThrow(() -> new ResourceNotFoundException("Sync configuration", webhookId));
+
+        Optional<PushEvent> push = PushEvent.parse(payload);
+
+        // A push to another branch is not this workspace's business. Without this
+        // check any branch would trigger a sync of the configured one, so pushing a
+        // draft would apply main — work nobody asked for, attributed to a push that
+        // did not contain it.
+        if (push.isPresent() && !push.get().movedBranch(configuration.getBranch())) {
+            return SyncOutcome.ignored(
+                    "Ignored a push to %s; this workspace follows %s."
+                            .formatted(push.get().branchName(), configuration.getBranch()));
+        }
+
+        // The commit, not the branch. See PushEvent for why the difference matters.
+        return runner.run(configuration, null, push.map(PushEvent::commitId).orElse(null));
     }
 
     /**

backend/src/main/java/com/devforge/sync/infrastructure/ArchiveDocumentSource.java

@@ -65,8 +65,9 @@ public ArchiveDocumentSource() {
     }
 
     @Override
-    public SourceSnapshot fetch(SyncConfiguration configuration, String accessToken) {
-        URI archive = archiveUri(configuration);
+    public SourceSnapshot fetch(SyncConfiguration configuration, String accessToken, String revision) {
+        String ref = revision == null || revision.isBlank() ? configuration.getBranch() : revision;
+        URI archive = archiveUri(configuration, ref);
 
         HttpRequest.Builder request = HttpRequest.newBuilder(archive)
                 .timeout(Duration.ofSeconds(60))
@@ -94,8 +95,8 @@ public SourceSnapshot fetch(SyncConfiguration configuration, String accessToken)
 
         if (response.statusCode() == 404) {
             throw new SourceUnavailableException(
-                    ("Nothing found at that repository and branch. Check the URL and that "
-                     + "branch '%s' exists.").formatted(configuration.getBranch()));
+                    ("Nothing found at that repository and revision. Check the URL and that "
+                     + "'%s' exists.").formatted(ref));
         }
         if (response.statusCode() == 401 || response.statusCode() == 403) {
             throw new SourceUnavailableException(
@@ -110,7 +111,9 @@ public SourceSnapshot fetch(SyncConfiguration configuration, String accessToken)
                             .formatted(response.statusCode()));
         }
 
-        return new SourceSnapshot(configuration.getBranch(), read(response.body()));
+        // The snapshot records what was actually read, so the activity log says
+        // which commit a change came from rather than repeating the branch name.
+        return new SourceSnapshot(ref, read(response.body()));
     }
 
     /**
@@ -135,7 +138,7 @@ private static boolean looksLikeZip(byte[] bytes) {
      * A trailing {@code .git} is stripped because that is how a clone URL is usually
      * copied, and it is not part of the web path.
      */
-    private static URI archiveUri(SyncConfiguration configuration) {
+    private static URI archiveUri(SyncConfiguration configuration, String ref) {
         String base = configuration.getRepositoryUrl().trim()
                 .replaceAll("/+$", "")
                 .replaceAll("\\.git$", "");
@@ -149,7 +152,6 @@ private static URI archiveUri(SyncConfiguration configuration) {
             // The ref goes in a path segment, so anything that could break out of it
             // is rejected rather than escaped — a branch name is not a place for
             // path traversal.
-            String ref = configuration.getBranch();
             if (ref.contains("..") || ref.startsWith("/")) {
                 throw new SourceUnavailableException("That branch name is not usable in a URL.");
             }

backend/src/test/java/com/devforge/sync/api/GitSyncIntegrationTest.java

@@ -19,6 +19,7 @@
 import java.util.UUID;
 
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete;
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.put;
@@ -58,13 +59,17 @@ static class ScriptedSource implements DocumentSource {
         List<SourceFile> files = new ArrayList<>();
         String ref = "abc1234";
         RuntimeException failure;
+        /** What the caller asked to read, so a test can assert on it. */
+        String requestedRevision;
 
         @Override
-        public SourceSnapshot fetch(SyncConfiguration configuration, String accessToken) {
+        public SourceSnapshot fetch(
+                SyncConfiguration configuration, String accessToken, String revision) {
+            this.requestedRevision = revision;
             if (failure != null) {
                 throw failure;
             }
-            return new SourceSnapshot(ref, List.copyOf(files));
+            return new SourceSnapshot(revision == null ? ref : revision, List.copyOf(files));
         }
     }
 
@@ -457,6 +462,111 @@ void appliesAPushDeliveredWithAValidSignature() throws Exception {
                 .andExpect(status().isOk());
     }
 
+    /** A signed delivery, as a git host sends one. */
+    private org.springframework.test.web.servlet.ResultActions deliver(
+            UUID webhookId, String secret, String payload) throws Exception {
+        return mockMvc.perform(post("/api/public/sync/{id}", webhookId)
+                .contentType("application/json")
+                .header("X-Hub-Signature-256",
+                        "sha256=" + WebhookSignature.hex(payload.getBytes(), secret))
+                .content(payload));
+    }
+
+    /**
+     * The defect this exists to prevent: a host serves the branch archive from a
+     * cache, and a webhook arrives the instant the push lands. Reading the branch
+     * can return the tree from before the push that triggered it, leaving the
+     * workspace one commit behind and looking healthy — because every push applies
+     * the previous one. Naming the commit is immune to that.
+     */
+    @Test
+    void readsTheCommitTheDeliveryNamedRatherThanTheBranch() throws Exception {
+        TestUser owner = registerUser("owner@acme.test", "Owner");
+        UUID ws = createWorkspace(owner, "Platform", "platform");
+        configure(owner, ws, "ARCHIVE");
+        String secret = generateSecret(owner, ws);
+        UUID webhookId = webhookIdOf(owner, ws);
+
+        source.files = List.of(new SourceFile("docs/pushed.md", "# Pushed"));
+
+        deliver(webhookId, secret,
+                "{\"ref\":\"refs/heads/main\",\"after\":\"9f8e7d6c5b4a3f2e1d0c\"}")
+                .andExpect(status().isOk())
+                .andExpect(jsonPath("$.created").value(1));
+
+        assertThat(source.requestedRevision).isEqualTo("9f8e7d6c5b4a3f2e1d0c");
+    }
+
+    /** And the commit is what history says the change came from. */
+    @Test
+    void recordsTheCommitAgainstTheSync() throws Exception {
+        TestUser owner = registerUser("owner@acme.test", "Owner");
+        UUID ws = createWorkspace(owner, "Platform", "platform");
+        configure(owner, ws, "ARCHIVE");
+        String secret = generateSecret(owner, ws);
+        UUID webhookId = webhookIdOf(owner, ws);
+
+        source.files = List.of(new SourceFile("docs/pushed.md", "# Pushed"));
+        deliver(webhookId, secret, "{\"ref\":\"refs/heads/main\",\"after\":\"deadbeef\"}");
+
+        // The whole record, not just the commit. A webhook sync used to apply its
+        // documents and record none of this, because the configuration it wrote to
+        // had come from another transaction and was no longer being tracked — so the
+        // sync worked and the settings screen said it had never run.
+        mockMvc.perform(authed(get("/api/workspaces/{w}/sync", ws), owner))
+                .andExpect(jsonPath("$.lastRef").value("deadbeef"))
+                .andExpect(jsonPath("$.lastStatus").value("OK"))
+                .andExpect(jsonPath("$.lastAttemptedAt").isNotEmpty())
+                .andExpect(jsonPath("$.lastCreated").value(1));
+    }
+
+    /**
+     * A repository is a working space. Without this, pushing a draft branch would
+     * sync the configured one — work nobody asked for, attributed to a push that
+     * did not contain it.
+     */
+    @Test
+    void ignoresAPushToABranchTheWorkspaceDoesNotFollow() throws Exception {
+        TestUser owner = registerUser("owner@acme.test", "Owner");
+        UUID ws = createWorkspace(owner, "Platform", "platform");
+        configure(owner, ws, "ARCHIVE");
+        String secret = generateSecret(owner, ws);
+        UUID webhookId = webhookIdOf(owner, ws);
+
+        source.files = List.of(new SourceFile("docs/pushed.md", "# Pushed"));
+
+        deliver(webhookId, secret,
+                "{\"ref\":\"refs/heads/draft\",\"after\":\"abc123\"}")
+                .andExpect(status().isOk())
+                // Not a failure: the delivery worked exactly as intended, and a red
+                // status here would be a repository with nothing wrong with it.
+                .andExpect(jsonPath("$.status").value("OK"))
+                .andExpect(jsonPath("$.created").value(0))
+                .andExpect(jsonPath("$.message").value(
+                        org.hamcrest.Matchers.containsString("Ignored a push to draft")));
+
+        mockMvc.perform(authed(get("/api/workspaces/{w}/documents/by-slug/{s}", ws, "pushed"), owner))
+                .andExpect(status().isNotFound());
+    }
+
+    /** A ping, or a host that sends something else: sync the branch, as before. */
+    @Test
+    void fallsBackToTheBranchWhenTheDeliveryNamesNoCommit() throws Exception {
+        TestUser owner = registerUser("owner@acme.test", "Owner");
+        UUID ws = createWorkspace(owner, "Platform", "platform");
+        configure(owner, ws, "ARCHIVE");
+        String secret = generateSecret(owner, ws);
+        UUID webhookId = webhookIdOf(owner, ws);
+
+        source.files = List.of(new SourceFile("docs/pushed.md", "# Pushed"));
+
+        deliver(webhookId, secret, "{\"zen\":\"Design for failure.\"}")
+                .andExpect(status().isOk())
+                .andExpect(jsonPath("$.created").value(1));
+
+        assertThat(source.requestedRevision).isNull();
+    }
+
     /** The security boundary of the whole feature. */
     @Test
     void refusesADeliveryWithTheWrongSignature() throws Exception {