fix(wiki): compare drift against what NodeBB stored, not what we rendered

The drift guard hashed NodeBB's stored copy of a page and compared it to the hash of the text Crucible rendered. Those only match if NodeBB round-trips our HTML byte for byte, so every sow-topdata deploy failed with "remote managed wiki content drifted; rerun with --force" on pages nobody had edited. Record the managed-region hash of the post NodeBB hands back right after each write (manifest field remote_hash) and compare the next run against that. A post with no sourceContent is no longer drift either: it predates sourceContent sync, so it reads back as rendered HTML and belongs to the existing repair path. The error now names the drifted pages, capped at ten. Old manifests without remote_hash keep the previous comparison, so no re-seed is needed. Costs one extra post read per page written; skipped pages are still never fetched. Closes #103 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-07 22:44:06 +02:00
parent 166ffb545e
commit 63a0f6f044
2 changed files with 320 additions and 12 deletions
@@ -62,8 +62,11 @@ type DeployResult struct {
 	Purged     int
 	Skipped    int
 	Drifted    int
-	Renamed    int
-	Manifest   string
+	// DriftedPages names the pages counted in Drifted, in plan order, so an
+	// operator can look at them before deciding whether --force is safe.
+	DriftedPages []string
+	Renamed      int
+	Manifest     string
 	// ResetPurged counts the deletions queued by --reset-managed-namespaces.
 	// They are also included in Stale and Purged, because callers warn about
 	// destructive policies in terms of the stale count: sow-topdata's
@@ -94,7 +97,13 @@ type wikiDeployManifest struct {
 }

 type wikiDeployManifestPage struct {
-	Hash                string `json:"hash"`
+	Hash string `json:"hash"`
+	// RemoteHash is the managed-region hash of the post body NodeBB handed back
+	// right after our last write to it. NodeBB owns that body, so this — not
+	// Hash, which is the hash of the text we rendered — is what the next run's
+	// drift check compares against. Empty on manifests written before this
+	// field existed; the drift check falls back to Hash for those.
+	RemoteHash          string `json:"remote_hash,omitempty"`
 	LastSeenHash        string `json:"last_seen_hash,omitempty"`
 	ArchivedHash        string `json:"archived_hash,omitempty"`
 	Title               string `json:"title,omitempty"`
@@ -108,12 +117,11 @@ type wikiDeployManifestPage struct {
 }

 type wikiDeployPlan struct {
-	Page       wikiDeployPage
-	Entry      wikiDeployManifestPage
-	Action     string
-	Content    string
-	RemoteHash string
-	Title      string
+	Page    wikiDeployPage
+	Entry   wikiDeployManifestPage
+	Action  string
+	Content string
+	Title   string
 	// Reset marks a purge queued by --reset-managed-namespaces rather than by
 	// stale computation over the manifest.
 	Reset bool
@@ -223,7 +231,7 @@ func DeployWikiWithOptions(p *project.Project, opts DeployWikiOptions, progress
 		return result, nil
 	}
 	if result.Drifted > 0 && !opts.Force {
-		return result, errors.New("remote managed wiki content drifted; rerun with --force to overwrite")
+		return result, fmt.Errorf("remote managed wiki content drifted on %d page(s) (%s); rerun with --force to overwrite", result.Drifted, summarizeDriftedPages(result.DriftedPages))
 	}

 	summary := p.EffectiveConfig().TopData.Wiki.DeployEditSummary
@@ -259,10 +267,16 @@ func DeployWikiWithOptions(p *project.Project, opts DeployWikiOptions, progress
 			entry.TID = created.TID
 			entry.PID = created.PID
 			nextManifest.Pages[plan.Page.PageID] = entry
+			if err := recordRemoteHash(nextManifest, plan.Page.PageID, created.PID, client); err != nil {
+				return result, err
+			}
 		case "update":
 			if err := client.updatePost(plan.Entry.TID, plan.Entry.PID, plan.Content, summary); err != nil {
 				return result, fmt.Errorf("deploy wiki page %q: update NodeBB post %d: %w", plan.Page.PageID, plan.Entry.PID, err)
 			}
+			if err := recordRemoteHash(nextManifest, plan.Page.PageID, plan.Entry.PID, client); err != nil {
+				return result, err
+			}
 		case "archive":
 			// Archiving rewrites the page rather than removing it, so it goes
 			// through the ordinary post edit the wiki plugin allows; only
@@ -680,8 +694,9 @@ func planNodeBBDeploy(pages map[string]wikiDeployPage, manifest wikiDeployManife
 			next.Pages[pageID] = entry
 		}
 		remoteHash := computeManagedHash(remote.Content)
-		if manifest.Pages[pageID].Hash != "" && remoteHash != manifest.Pages[pageID].Hash {
+		if remoteContentDrifted(manifest.Pages[pageID], remote, remoteHash) {
 			result.Drifted++
+			result.DriftedPages = append(result.DriftedPages, pageID)
 			if !opts.Force {
 				continue
 			}
@@ -694,7 +709,7 @@ func planNodeBBDeploy(pages map[string]wikiDeployPage, manifest wikiDeployManife
 		result.Updated++
 		entry.SourceContentSynced = true
 		next.Pages[pageID] = entry
-		plans = append(plans, wikiDeployPlan{Page: page, Entry: entry, Action: "update", Content: merged, RemoteHash: remoteHash})
+		plans = append(plans, wikiDeployPlan{Page: page, Entry: entry, Action: "update", Content: merged})
 	}
 	for pageID := range manifest.Pages {
 		if _, ok := pages[pageID]; !ok {
@@ -869,6 +884,9 @@ func recoverCreateCollision(plan wikiDeployPlan, manifest wikiDeployManifest, cl
 	entry.Stale = false
 	entry.SourceContentSynced = true
 	manifest.Pages[plan.Page.PageID] = entry
+	if err := recordRemoteHash(manifest, plan.Page.PageID, entry.PID, client); err != nil {
+		return false, err
+	}
 	return true, nil
 }

@@ -1206,6 +1224,49 @@ func saveDeployManifest(path string, manifest wikiDeployManifest) error {
 	return os.WriteFile(path, append(raw, '\n'), 0o644)
 }

+// summarizeDriftedPages names the drifted pages for the operator, capped so a
+// mass-drift run does not bury the rest of the CI log.
+func summarizeDriftedPages(pages []string) string {
+	const maxNamed = 10
+	if len(pages) <= maxNamed {
+		return strings.Join(pages, ", ")
+	}
+	return fmt.Sprintf("%s and %d more", strings.Join(pages[:maxNamed], ", "), len(pages)-maxNamed)
+}
+
+// remoteContentDrifted answers the only question the drift guard exists to ask:
+// did the live page change after we last wrote it? It is not "does the live
+// page differ from what we just rendered" — NodeBB owns the stored body, and a
+// deploy that regenerates a page always differs from what is live.
+func remoteContentDrifted(entry wikiDeployManifestPage, remote nodeBBPost, remoteHash string) bool {
+	// A post with no sourceContent was written before the deployer stored one,
+	// so what comes back is NodeBB's rendered HTML and can never hash-equal
+	// anything we wrote. That is a page awaiting the SourceContentSynced
+	// repair, not a human edit.
+	if remote.SourceContent == "" {
+		return false
+	}
+	if entry.RemoteHash != "" {
+		return remoteHash != entry.RemoteHash
+	}
+	// Manifest written before RemoteHash existed: fall back to the old
+	// comparison rather than declaring every tracked page drifted at once.
+	return entry.Hash != "" && remoteHash != entry.Hash
+}
+
+// recordRemoteHash reads a page back after writing it and records what NodeBB
+// actually stored, so the next run compares remote against remote.
+func recordRemoteHash(manifest wikiDeployManifest, pageID string, pid int, client *nodeBBClient) error {
+	remote, err := client.getPost(pid)
+	if err != nil {
+		return fmt.Errorf("deploy wiki page %q: read back NodeBB post %d: %w", pageID, pid, err)
+	}
+	entry := manifest.Pages[pageID]
+	entry.RemoteHash = computeManagedHash(remote.Content)
+	manifest.Pages[pageID] = entry
+	return nil
+}
+
 func computeManagedHash(content string) string {
 	managed, ok := extractManagedRegion(content)
 	if !ok {
@@ -741,6 +741,10 @@ func TestDeployWikiCreatesNodeBBTopicAndWritesManifest(t *testing.T) {
 			_ = json.NewEncoder(w).Encode(map[string]any{"response": map[string]any{"pages": []any{}, "hasMore": false}})
 			return
 		}
+		if r.Method == http.MethodGet && r.URL.Path == "/api/v3/posts/42" {
+			respondWikiPost(w, 42, 11, generated)
+			return
+		}
 		if r.Method != http.MethodPost || r.URL.Path != "/api/v3/topics" {
 			t.Fatalf("unexpected request %s %s", r.Method, r.URL.Path)
 		}
@@ -796,6 +800,9 @@ func TestDeployWikiCreatesNodeBBTopicAndWritesManifest(t *testing.T) {
 	if !entry.SourceContentSynced {
 		t.Fatalf("expected created manifest entry to record sourceContent sync")
 	}
+	if entry.RemoteHash != computeManagedHash(generated) {
+		t.Fatalf("expected created manifest entry to record the read-back remote hash, got %#v", entry)
+	}
 }

 func TestParseNodeBBPostPrefersSourceContentForWikiHTML(t *testing.T) {
@@ -958,6 +965,10 @@ func TestDeployWikiCreatesNodeBBTopicWithoutFallbackForDefaultThreeCharacterTitl
 			_ = json.NewEncoder(w).Encode(map[string]any{"response": map[string]any{"pages": []any{}, "hasMore": false}})
 			return
 		}
+		if r.Method == http.MethodGet && r.URL.Path == "/api/v3/posts/42" {
+			respondWikiPost(w, 42, 11, generated)
+			return
+		}
 		if r.Method != http.MethodPost || r.URL.Path != "/api/v3/topics" {
 			t.Fatalf("unexpected request %s %s", r.Method, r.URL.Path)
 		}
@@ -1017,6 +1028,10 @@ func TestDeployWikiCreatesNodeBBTopicWithFallbackForTitleShorterThanConfiguredMi
 			_ = json.NewEncoder(w).Encode(map[string]any{"response": map[string]any{"pages": []any{}, "hasMore": false}})
 			return
 		}
+		if r.Method == http.MethodGet && r.URL.Path == "/api/v3/posts/42" {
+			respondWikiPost(w, 42, 11, generated)
+			return
+		}
 		if r.Method != http.MethodPost || r.URL.Path != "/api/v3/topics" {
 			t.Fatalf("unexpected request %s %s", r.Method, r.URL.Path)
 		}
@@ -2081,6 +2096,8 @@ func TestDeployWikiPurgesTrackedStaleTopicsBeforeCreatingReplacementPages(t *tes
 		case r.Method == http.MethodGet && r.URL.Path == "/api/v3/plugins/westgate-wiki/namespace/3/pages":
 			w.Header().Set("Content-Type", "application/json")
 			_ = json.NewEncoder(w).Encode(map[string]any{"response": map[string]any{"pages": []any{}, "hasMore": false}})
+		case r.Method == http.MethodGet && r.URL.Path == "/api/v3/posts/99":
+			respondWikiPost(w, 99, 11, generated)
 		case r.Method == http.MethodPost && r.URL.Path == "/api/v3/topics":
 			calls = append(calls, "create")
 			if strings.Join(calls, ",") != "tombstone:7,hard-purge:7,create" {
@@ -2165,6 +2182,8 @@ func TestDeployWikiResetManagedNamespacesPurgesRemotePagesBeforeCreatingFreshMan
 					"hasMore": false,
 				},
 			})
+		case r.Method == http.MethodGet && r.URL.Path == "/api/v3/posts/99":
+			respondWikiPost(w, 99, 11, generated)
 		case r.Method == http.MethodPost && r.URL.Path == "/api/v3/topics":
 			calls = append(calls, "create")
 			if strings.Join(calls, ",") != resetCalls+",create" {
@@ -2399,6 +2418,234 @@ func respondWikiEditLock(t *testing.T, w http.ResponseWriter, r *http.Request, e
 // goes through here, so a deployer that reaches for the core topic API fails in
 // CI the way it fails in production rather than passing against a fake that is
 // more permissive than the real thing.
+// driftScenario is one managed page whose generated text changed since the last
+// deploy, so the deployer has to read the live post and rule on drift. The fake
+// NodeBB serves whatever it was last told to store, which is what a real deploy
+// reads back after a write.
+type driftScenario struct {
+	sourceDir    string
+	manifestPath string
+	endpoint     string
+	generated    string
+	stored       string
+	updateCalls  int
+	lastWritten  string
+}
+
+// newDriftScenario writes the local page and the manifest entry. remoteStored is
+// the body NodeBB hands back; hasSourceContent says whether the post carries the
+// sourceContent the wiki plugin stores.
+func newDriftScenario(t *testing.T, entry wikiDeployManifestPage, remoteStored string, hasSourceContent bool) (*driftScenario, *httptest.Server) {
+	t.Helper()
+	root := t.TempDir()
+	sourceDir := filepath.Join(root, "pages")
+	if err := os.MkdirAll(filepath.Join(sourceDir, "skills"), 0755); err != nil {
+		t.Fatalf("create source dir: %v", err)
+	}
+	generated := `<!-- sow-topdata-wiki:page=skills:athletics -->
+<!-- sow-topdata-wiki:managed:start hash="sha256:local" -->
+<h1>Athletics</h1>
+<p>Generated athletics page</p>
+<!-- sow-topdata-wiki:managed:end -->
+`
+	if err := os.WriteFile(filepath.Join(sourceDir, "skills", "athletics.html"), []byte(generated), 0644); err != nil {
+		t.Fatalf("write source page: %v", err)
+	}
+	manifestPath := filepath.Join(root, "deploy-manifest.json")
+	entry.TID, entry.PID, entry.CID = 7, 42, 3
+	entry.SourceContentSynced = true
+	if err := saveDeployManifest(manifestPath, wikiDeployManifest{
+		Version: "nodebb-v1",
+		Pages:   map[string]wikiDeployManifestPage{"skills:athletics": entry},
+	}); err != nil {
+		t.Fatalf("write deploy manifest: %v", err)
+	}
+
+	scenario := &driftScenario{sourceDir: sourceDir, manifestPath: manifestPath, generated: generated, stored: remoteStored}
+	server := newFakeNodeBB(t, func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case r.Method == http.MethodGet && r.URL.Path == "/api/v3/posts/42":
+			post := map[string]any{"pid": 42, "tid": 7, "content": scenario.stored}
+			if hasSourceContent {
+				post["sourceContent"] = scenario.stored
+			}
+			w.Header().Set("Content-Type", "application/json")
+			_ = json.NewEncoder(w).Encode(map[string]any{"response": post})
+		case r.Method == http.MethodPut && r.URL.Path == "/api/v3/plugins/westgate-wiki/edit-lock":
+			respondWikiEditLock(t, w, r, 7, "drift-lock")
+		case r.Method == http.MethodPut && r.URL.Path == "/api/v3/posts/42":
+			var req struct {
+				Content string `json:"content"`
+			}
+			if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+				t.Fatalf("decode update request: %v", err)
+			}
+			scenario.updateCalls++
+			scenario.lastWritten = req.Content
+			// NodeBB owns the stored body from here on, and the read-back that
+			// follows has to see it.
+			scenario.stored = req.Content
+			hasSourceContent = true
+			w.Header().Set("Content-Type", "application/json")
+			_ = json.NewEncoder(w).Encode(map[string]any{"response": map[string]any{"pid": 42, "tid": 7}})
+		default:
+			t.Fatalf("unexpected request %s %s", r.Method, r.URL.Path)
+		}
+	})
+	return scenario, server
+}
+
+func (s *driftScenario) deploy(t *testing.T, force bool) (DeployResult, error) {
+	t.Helper()
+	return DeployWikiWithOptions(&project.Project{}, DeployWikiOptions{
+		SourceDir:    s.sourceDir,
+		Endpoint:     s.endpoint,
+		Token:        "nodebb-token",
+		ManifestPath: s.manifestPath,
+		Force:        force,
+	}, nil)
+}
+
+func TestDeployWikiDoesNotTreatNodeBBNormalizationAsDrift(t *testing.T) {
+	// NodeBB stores its own copy of the page, so what it hands back never has
+	// to be byte-equal to the text we rendered. Only a change since our last
+	// write is drift.
+	stored := `<!-- sow-topdata-wiki:page=skills:athletics -->
+<!-- sow-topdata-wiki:managed:start hash="sha256:local" -->
+<h1>Athletics</h1><p>Old generated text</p>
+<!-- sow-topdata-wiki:managed:end -->
+`
+	scenario, server := newDriftScenario(t, wikiDeployManifestPage{
+		Hash:       "sha256:whatever-we-rendered-last-time",
+		RemoteHash: computeManagedHash(stored),
+	}, stored, true)
+	defer server.Close()
+	scenario.endpoint = server.URL
+
+	result, err := scenario.deploy(t, false)
+	if err != nil {
+		t.Fatalf("expected normalized remote copy to deploy cleanly, got: %v", err)
+	}
+	if result.Drifted != 0 || result.Updated != 1 || scenario.updateCalls != 1 {
+		t.Fatalf("expected a clean update, got %#v (updateCalls=%d)", result, scenario.updateCalls)
+	}
+	entry := loadDeployManifest(scenario.manifestPath).Pages["skills:athletics"]
+	if entry.RemoteHash != computeManagedHash(scenario.lastWritten) {
+		t.Fatalf("expected manifest to record the read-back remote hash, got %#v", entry)
+	}
+}
+
+func TestDeployWikiReportsEditedPageAsDriftedAndNamesIt(t *testing.T) {
+	stored := `<!-- sow-topdata-wiki:page=skills:athletics -->
+<!-- sow-topdata-wiki:managed:start hash="sha256:local" -->
+<h1>Athletics</h1>
+<p>A person rewrote this by hand.</p>
+<!-- sow-topdata-wiki:managed:end -->
+`
+	scenario, server := newDriftScenario(t, wikiDeployManifestPage{
+		Hash:       "sha256:whatever-we-rendered-last-time",
+		RemoteHash: "sha256:what-we-wrote-last-time",
+	}, stored, true)
+	defer server.Close()
+	scenario.endpoint = server.URL
+
+	result, err := scenario.deploy(t, false)
+	if err == nil {
+		t.Fatalf("expected a hand-edited page to block the deploy, got %#v", result)
+	}
+	if !strings.Contains(err.Error(), "skills:athletics") {
+		t.Fatalf("expected the drift error to name the page, got: %v", err)
+	}
+	if result.Drifted != 1 || scenario.updateCalls != 0 {
+		t.Fatalf("expected one drifted page and no write, got %#v (updateCalls=%d)", result, scenario.updateCalls)
+	}
+}
+
+func TestDeployWikiForceOverwritesDriftedPageAndRerecordsRemoteHash(t *testing.T) {
+	stored := `<!-- sow-topdata-wiki:page=skills:athletics -->
+<!-- sow-topdata-wiki:managed:start hash="sha256:local" -->
+<h1>Athletics</h1>
+<p>A person rewrote this by hand.</p>
+<!-- sow-topdata-wiki:managed:end -->
+`
+	scenario, server := newDriftScenario(t, wikiDeployManifestPage{
+		Hash:       "sha256:whatever-we-rendered-last-time",
+		RemoteHash: "sha256:what-we-wrote-last-time",
+	}, stored, true)
+	defer server.Close()
+	scenario.endpoint = server.URL
+
+	result, err := scenario.deploy(t, true)
+	if err != nil {
+		t.Fatalf("expected --force to overwrite the drifted page, got: %v", err)
+	}
+	if result.Drifted != 1 || result.Updated != 1 || scenario.updateCalls != 1 {
+		t.Fatalf("expected the drifted page overwritten once, got %#v (updateCalls=%d)", result, scenario.updateCalls)
+	}
+	entry := loadDeployManifest(scenario.manifestPath).Pages["skills:athletics"]
+	if entry.RemoteHash != computeManagedHash(scenario.lastWritten) {
+		t.Fatalf("expected the forced write to re-record the remote hash, got %#v", entry)
+	}
+}
+
+func TestDeployWikiTreatsPostWithoutSourceContentAsUnsyncedNotDrifted(t *testing.T) {
+	// A post written before the deployer stored sourceContent reads back as
+	// NodeBB's rendered HTML, which can never match anything we wrote. That is
+	// a page waiting for the sourceContent repair, not a hand edit.
+	rendered := "&lt;h1&gt;Athletics&lt;/h1&gt;&lt;p&gt;Old generated text&lt;/p&gt;"
+	scenario, server := newDriftScenario(t, wikiDeployManifestPage{
+		Hash: "sha256:whatever-we-rendered-last-time",
+	}, rendered, false)
+	defer server.Close()
+	scenario.endpoint = server.URL
+
+	result, err := scenario.deploy(t, false)
+	if err != nil {
+		t.Fatalf("expected an unsynced page to deploy, got: %v", err)
+	}
+	if result.Drifted != 0 || result.Updated != 1 || scenario.updateCalls != 1 {
+		t.Fatalf("expected one repair update and no drift, got %#v (updateCalls=%d)", result, scenario.updateCalls)
+	}
+}
+
+func TestDeployWikiFallsBackToLocalHashForManifestWithoutRemoteHash(t *testing.T) {
+	// Manifest written before remote_hash existed: the remote still matches the
+	// recorded local hash, so nothing drifted, and this run records the remote
+	// hash for the next one.
+	stored := `<!-- sow-topdata-wiki:page=skills:athletics -->
+<!-- sow-topdata-wiki:managed:start hash="sha256:old" -->
+<h1>Athletics</h1>
+<p>Old generated text</p>
+<!-- sow-topdata-wiki:managed:end -->
+`
+	scenario, server := newDriftScenario(t, wikiDeployManifestPage{
+		Hash: computeManagedHash(stored),
+	}, stored, true)
+	defer server.Close()
+	scenario.endpoint = server.URL
+
+	result, err := scenario.deploy(t, false)
+	if err != nil {
+		t.Fatalf("expected an old-format manifest to deploy, got: %v", err)
+	}
+	if result.Drifted != 0 || result.Updated != 1 {
+		t.Fatalf("expected a clean update, got %#v", result)
+	}
+	entry := loadDeployManifest(scenario.manifestPath).Pages["skills:athletics"]
+	if entry.RemoteHash == "" {
+		t.Fatalf("expected the deploy to record a remote hash, got %#v", entry)
+	}
+}
+
+// respondWikiPost answers a post read the way the wiki plugin does: the stored
+// source HTML in sourceContent, alongside the body NodeBB renders from it.
+func respondWikiPost(w http.ResponseWriter, pid, tid int, content string) {
+	w.Header().Set("Content-Type", "application/json")
+	_ = json.NewEncoder(w).Encode(map[string]any{"response": map[string]any{
+		"pid": pid, "tid": tid, "content": content, "sourceContent": content,
+	}})
+}
+
 func newFakeNodeBB(t *testing.T, handler http.HandlerFunc) *httptest.Server {
 	t.Helper()
 	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {