From b165e5fe7aaa9ca2a6824f02b990fdb7fe0dffd8 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 24 Apr 2026 19:13:03 -0400 Subject: [PATCH] fix(server): summarise structured field changes in activity feed (BUG-748) (#236) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 | 59 +++++++- internal/server/handlers_documents_test.go | 164 +++++++++++++++++++++ 2 files changed, 217 insertions(+), 6 deletions(-) create mode 100644 internal/server/handlers_documents_test.go diff --git a/internal/server/handlers_documents.go b/internal/server/handlers_documents.go index 2da1b3ad..569f12f5 100644 --- a/internal/server/handlers_documents.go +++ b/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) + } +} diff --git a/internal/server/handlers_documents_test.go b/internal/server/handlers_documents_test.go new file mode 100644 index 00000000..42de50b6 --- /dev/null +++ b/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) + } +}