fix(server): summarise structured field changes in activity feed (BUG-748) (#236)

* fix(server): summarise structured field changes in activity feed (BUG-748)

The activity-feed `metadata.changes` string is built by `diffFields()` in
`handlers_documents.go`, which used `fmt.Sprintf("%v", val)` to stringify
each old/new value. For structured fields (implementation_notes,
decision_log, or any other slice/map value in item.fields) Go's default
formatting dumps the raw map repr — e.g.
  implementation_notes: → [map[created_at:2026-04-23T... details:Code audit
  on 2026-04-23 found Phases 1, 2, and most of Phase 3 already implemented:
  - **Phase 1a** ... created_by:user summary:Phases 1-3 verified shipped]]

Activity cards on the item detail page surfaced this verbatim, leaking
internal field shape into the UI.

Replace the bare `%v` with a `formatChangeValue` helper:

  - Primitives (string/number/bool): unchanged Go default formatting.
  - Slices: counted summary. Known fields get domain-specific phrasing
    (`(1 note)` / `(N notes)` for implementation_notes, `(1 entry)` /
    `(N entries)` for decision_log); unknown fields fall back to
    `(N items)`.
  - Maps/objects: `(object)` placeholder.
  - nil: empty string.

This is a backend-only change. The frontend `TimelineActivityCard.svelte`
keeps splitting on `→` exactly as before, so the contract is unchanged
beyond the value-formatting.

Companion to PR #235 (frontend `.prose` class fix on TimelineCommentCard).
Together they close BUG-748 — markdown content was unrenderable both
in plain timeline comments AND in activity-feed change pills that
referenced structured field updates.

Tests: 9 new cases in handlers_documents_test.go covering primitives,
added/removed fields, implementation_notes single + plural, decision_log
single + plural, generic slice fallback, object fallback, invalid JSON,
and nil safety. All green.

Verified: go build ./..., go vet ./..., go test ./..., web/npm run build
all clean.

* fix(server): compare values, not display strings, in diffFields (Codex round 1)

Codex flagged two MEDIUM regressions in PR #236 round 1:

1. Object-valued fields (e.g. `convention`, `github_pr`) all stringify to
   the same `(object)` label, so an in-place edit produced
   oldStr == newStr == "(object)" and `diffFields()` silently dropped the
   change from `metadata.changes` — the activity card stopped recording
   that the field had been edited.

2. Same problem for slice fields when length is unchanged: replacing one
   `implementation_note` with a different one (`{"summary":"original"}`
   → `{"summary":"revised"}`) collapsed both sides to `(1 note)` and
   the change vanished from the activity log.

Switch the equality check from string-on-display to `reflect.DeepEqual`
on the raw decoded values. The display strings still go through
`formatChangeValue()` so the activity card stays clean (`(1 note) → (1
note)` for a same-cardinality replacement is coarse but correct — the
user knows something changed and can drill in via the timeline). For
truly identical values the entry is omitted, so no false positives.

`reflect.DeepEqual` is correct for the types `json.Unmarshal` into
`map[string]any` produces: nil, bool, float64, string, []any,
map[string]any.

New tests:
- TestDiffFieldsSameCardinalityArrayChangeStillReported
- TestDiffFieldsObjectMutationStillReported
Each also asserts the no-op case (identical input on both sides emits
nothing).

Verified: go build ./..., go vet ./..., go test -count=1 ./... all green.

internal/server/handlers_documents.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"reflect"
 	"sort"
 	"strings"
 
@@ -568,18 +569,64 @@ func diffFields(oldFields, newFields string) string {
 	var changes []string
 	for key, newVal := range newMap {
 		oldVal, exists := oldMap[key]
-		newStr := fmt.Sprintf("%v", newVal)
+		newStr := formatChangeValue(key, newVal)
 		if !exists {
 			changes = append(changes, fmt.Sprintf("%s: → %s", key, newStr))
-		} else {
-			oldStr := fmt.Sprintf("%v", oldVal)
-			if oldStr != newStr {
-				changes = append(changes, fmt.Sprintf("%s: %s → %s", key, oldStr, newStr))
-			}
+			continue
 		}
+		// Compare on the raw decoded values rather than the display strings.
+		// formatChangeValue() collapses structured values to fixed labels
+		// (e.g. `(1 note)`, `(object)`), so two semantically distinct edits
+		// of the same cardinality would otherwise produce equal display
+		// strings and the change would be silently dropped from the activity
+		// metadata. reflect.DeepEqual is correct for the types `json.Unmarshal`
+		// produces here (nil, bool, float64, string, []any, map[string]any).
+		if reflect.DeepEqual(oldVal, newVal) {
+			continue
+		}
+		oldStr := formatChangeValue(key, oldVal)
+		changes = append(changes, fmt.Sprintf("%s: %s → %s", key, oldStr, newStr))
 	}
 
 	// Sort for deterministic output
 	sort.Strings(changes)
 	return strings.Join(changes, ", ")
 }
+
+// formatChangeValue renders a JSON-decoded field value as a human-readable
+// string for the activity-feed `changes` summary. Primitives use Go's default
+// formatting; structured values (slices, maps) are summarised as a count or
+// short label so the activity card never surfaces Go's `[map[k:v ...]]`
+// repr to end users (BUG-748).
+//
+// Known structured fields (`implementation_notes`, `decision_log`) get
+// domain-specific phrasing; unknown structured fields fall back to a generic
+// `(N items)` / `(object)` label.
+func formatChangeValue(field string, val any) string {
+	if val == nil {
+		return ""
+	}
+	switch v := val.(type) {
+	case []any:
+		switch field {
+		case models.ItemFieldImplementationNotes:
+			if len(v) == 1 {
+				return "(1 note)"
+			}
+			return fmt.Sprintf("(%d notes)", len(v))
+		case models.ItemFieldDecisionLog:
+			if len(v) == 1 {
+				return "(1 entry)"
+			}
+			return fmt.Sprintf("(%d entries)", len(v))
+		}
+		if len(v) == 1 {
+			return "(1 item)"
+		}
+		return fmt.Sprintf("(%d items)", len(v))
+	case map[string]any:
+		return "(object)"
+	default:
+		return fmt.Sprintf("%v", v)
+	}
+}

internal/server/handlers_documents_test.go

@@ -0,0 +1,164 @@
+package server
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestDiffFieldsPrimitives(t *testing.T) {
+	got := diffFields(
+		`{"status":"open","priority":"medium"}`,
+		`{"status":"in-progress","priority":"high"}`,
+	)
+	// Order is alphabetical (sort.Strings on the changes slice).
+	want := "priority: medium → high, status: open → in-progress"
+	if got != want {
+		t.Errorf("diffFields primitives:\n  got:  %q\n  want: %q", got, want)
+	}
+}
+
+func TestDiffFieldsAddedRemovedFields(t *testing.T) {
+	// Newly added field — old map missing the key.
+	got := diffFields(`{"status":"open"}`, `{"status":"open","priority":"high"}`)
+	want := "priority: → high"
+	if got != want {
+		t.Errorf("diffFields added:\n  got:  %q\n  want: %q", got, want)
+	}
+}
+
+func TestDiffFieldsImplementationNotesSummarised(t *testing.T) {
+	// Add the first implementation note. Without the structured-field
+	// formatting this used to render as
+	//   `implementation_notes: → [map[created_at:... details:... summary:...]]`
+	// which is the BUG-748 activity-card regression we're guarding against.
+	old := `{"status":"open"}`
+	updated := `{"status":"open","implementation_notes":[{"id":"n1","summary":"Phases 1-3 verified shipped","details":"Code audit on 2026-04-23 found Phase 1, 2, and most of Phase 3 already implemented","created_at":"2026-04-23T00:49:04Z","created_by":"user"}]}`
+
+	got := diffFields(old, updated)
+	want := "implementation_notes: → (1 note)"
+	if got != want {
+		t.Errorf("diffFields implementation_notes single:\n  got:  %q\n  want: %q", got, want)
+	}
+
+	// Confirm the raw map repr never leaks through.
+	if strings.Contains(got, "map[") || strings.Contains(got, "details:") {
+		t.Errorf("diffFields leaked Go map repr: %q", got)
+	}
+}
+
+func TestDiffFieldsImplementationNotesPluralised(t *testing.T) {
+	old := `{"implementation_notes":[{"summary":"first"}]}`
+	updated := `{"implementation_notes":[{"summary":"first"},{"summary":"second"},{"summary":"third"}]}`
+
+	got := diffFields(old, updated)
+	want := "implementation_notes: (1 note) → (3 notes)"
+	if got != want {
+		t.Errorf("diffFields implementation_notes count change:\n  got:  %q\n  want: %q", got, want)
+	}
+}
+
+func TestDiffFieldsDecisionLogSummarised(t *testing.T) {
+	old := `{"status":"active"}`
+	updated := `{"status":"active","decision_log":[{"id":"d1","decision":"Store notes in reserved field keys","rationale":"Avoid a new table"}]}`
+
+	got := diffFields(old, updated)
+	want := "decision_log: → (1 entry)"
+	if got != want {
+		t.Errorf("diffFields decision_log single:\n  got:  %q\n  want: %q", got, want)
+	}
+
+	old2 := `{"decision_log":[{"decision":"first"}]}`
+	updated2 := `{"decision_log":[{"decision":"first"},{"decision":"second"}]}`
+	got2 := diffFields(old2, updated2)
+	want2 := "decision_log: (1 entry) → (2 entries)"
+	if got2 != want2 {
+		t.Errorf("diffFields decision_log count change:\n  got:  %q\n  want: %q", got2, want2)
+	}
+}
+
+func TestDiffFieldsGenericStructuredFieldsFallback(t *testing.T) {
+	// An unknown array-of-objects field should still produce a summary,
+	// not a raw map repr.
+	old := `{"status":"open"}`
+	updated := `{"status":"open","custom_attachments":[{"name":"a.png"},{"name":"b.png"}]}`
+
+	got := diffFields(old, updated)
+	want := "custom_attachments: → (2 items)"
+	if got != want {
+		t.Errorf("diffFields generic structured field:\n  got:  %q\n  want: %q", got, want)
+	}
+	if strings.Contains(got, "map[") {
+		t.Errorf("diffFields generic structured field leaked Go map repr: %q", got)
+	}
+}
+
+func TestDiffFieldsObjectFieldFallback(t *testing.T) {
+	// A bare object field — also previously dumped as `map[k:v]`.
+	old := `{"status":"open"}`
+	updated := `{"status":"open","convention":{"trigger":"on-implement","scope":"all"}}`
+
+	got := diffFields(old, updated)
+	want := "convention: → (object)"
+	if got != want {
+		t.Errorf("diffFields object field:\n  got:  %q\n  want: %q", got, want)
+	}
+}
+
+func TestDiffFieldsHandlesInvalidJSON(t *testing.T) {
+	if got := diffFields("not-json", `{"a":1}`); got != "" {
+		t.Errorf("diffFields invalid old: expected empty, got %q", got)
+	}
+	if got := diffFields(`{"a":1}`, "not-json"); got != "" {
+		t.Errorf("diffFields invalid new: expected empty, got %q", got)
+	}
+}
+
+func TestFormatChangeValueNilSafe(t *testing.T) {
+	if got := formatChangeValue("status", nil); got != "" {
+		t.Errorf("formatChangeValue(nil): expected empty, got %q", got)
+	}
+}
+
+// Regression for the Codex-round-1 finding on PR #236: collapsing slice
+// values to a count-only label meant a same-cardinality replacement (one
+// note swapped for a different one note) produced equal old/new display
+// strings, so `diffFields()` silently dropped the change. The fix compares
+// the decoded values via reflect.DeepEqual instead of the display strings,
+// so a content change still produces a metadata.changes entry. The display
+// string is still the friendly summary (`(1 note) → (1 note)`) — informative
+// but coarse — which is acceptable: the activity card also shows the actor
+// + timestamp, so the user knows something changed and can drill in.
+func TestDiffFieldsSameCardinalityArrayChangeStillReported(t *testing.T) {
+	old := `{"implementation_notes":[{"id":"n1","summary":"original"}]}`
+	updated := `{"implementation_notes":[{"id":"n1","summary":"revised"}]}`
+
+	got := diffFields(old, updated)
+	want := "implementation_notes: (1 note) → (1 note)"
+	if got != want {
+		t.Errorf("diffFields same-cardinality replacement:\n  got:  %q\n  want: %q", got, want)
+	}
+
+	// Also verify same JSON on both sides produces no entry (true no-op).
+	if got := diffFields(old, old); got != "" {
+		t.Errorf("diffFields no-op: expected empty, got %q", got)
+	}
+}
+
+// Regression for the Codex-round-1 finding on PR #236: object-valued fields
+// (e.g. `convention`) now both stringify to `(object)`, so an in-place edit
+// would have been silently dropped. Confirm DeepEqual catches it.
+func TestDiffFieldsObjectMutationStillReported(t *testing.T) {
+	old := `{"convention":{"trigger":"on-implement","scope":"all"}}`
+	updated := `{"convention":{"trigger":"on-commit","scope":"all"}}`
+
+	got := diffFields(old, updated)
+	want := "convention: (object) → (object)"
+	if got != want {
+		t.Errorf("diffFields object mutation:\n  got:  %q\n  want: %q", got, want)
+	}
+
+	// Identical object — no-op, no entry.
+	if got := diffFields(old, old); got != "" {
+		t.Errorf("diffFields object no-op: expected empty, got %q", got)
+	}
+}