[bfs2021]

Problem

A record created or changed by applying a suggestion does not appear in search (Cmd/Ctrl+K, the Links tab picker, # mentions) until the organization's search index is next rebuilt. A record created through the equivalent HTTP endpoint appears straight away. With searchIndexTTL at 5 minutes, that means an apply stays invisible for up to 5+ minutes, a restart, or until a post-rebuild search — and everything an AI agent proposes and a manager applies lands in that blind spot.

Fixes #500

Root cause

Search runs on a per-org in-memory index (internal/isms/api/api_search.go). The create, update and delete HTTP handlers patch it directly (handleAddRisk → s.searchIndex.Upsert(orgID, riskSearchEntry(&r)), etc.), but the suggestion apply handlers (applyRiskCreate, applyRiskUpdate, applySupplierCreate and the rest registered in api_suggestions.go) write the record and never touch the index — the only search call there was the task removal for private tasks. When the index is stale but present, a search serves old results while rebuilding in the background, so the applied record only shows up on a search made after a rebuild has finished.

Fix

After a successful apply, upsert the applied record's search entry — post-commit via registerApplyPostCommit (as the private-task removal already does), so a rolled-back apply never reaches the index:

• every create/update apply handler now registers s.searchIndex.Upsert(orgID, <type>SearchEntry(&entity)), the same builder its HTTP counterpart uses (riskSearchEntry, systemSearchEntry, supplierSearchEntry, legalSearchEntry, auditFindingSearchEntry);
• the types that still built their search text inline got a builder first (incidentSearchEntry, assetSearchEntry, taskSearchEntry, changeSearchEntry, correctiveActionSearchEntry), and buildSearchIndex now uses them, plus single-item objectiveSearchEntry / programSearchEntry, so the rebuild path and the upsert path cannot drift;
• task visibility follows the HTTP handlers: a public task is upserted, a private task never enters the shared index (Task visibility: per-task private flag + org default #178), and a public→private flip still removes the entry.

All twelve entity types with a create or update apply handler are covered: risk, supplier, system, asset, legal requirement, task, incident, change, corrective action, objective, program and audit finding. Readings, reassessments and review suggestions change nothing the index holds, so they stay untouched.

Testing

New internal/isms/api/api_suggestions_search_pg_test.go (the database-backed style of change_followup_task_pg_test.go, skipped without ISMS_TEST_DATABASE_URL):

• TestSuggestionApplyUpdatesSearchIndex — fresh org, index built first, then a risk created by a suggestion is found by title straight away (and carries its RISK-n identifier), a rename applied through an update suggestion is found under the new title and no longer under the old one, and a supplier created by a suggestion is found too.
• TestSuggestionApplyTaskSearchVisibility — a public task created by a suggestion is searchable right away; a private one never enters the index.

Both fail on master (0 results after apply) and pass with the fix, against postgres:17 with the migrations applied — the same setup CI's "Database round-trip tests" step uses. go vet, gofmt -l . clean; the full go test ./internal/isms/... run shows no new failures versus baseline (on Windows the review git-store tests and TestValidateUpdatePayload fail identically before and after this change — a pre-existing environment/message-format issue unrelated to this fix; go build ./cmd/isms also does not compile on Windows even on master, as serve.go uses Unix-only syscalls).

[unidoc-alip]

Verdict: COMMENT — 0 must-fix, 2 should-fix, 1 nit

This PR makes records created or changed by applying a suggestion show up in search straight away. Each create/update apply handler now registers a post-commit searchIndex.Upsert built with the same per-type builder that buildSearchIndex uses. Five types that built their search text inline (incident, asset, task, change, corrective action) get a builder, and objectives and programs get single-item builders.

The fix is correct and complete for the apply path. I checked it against a real Postgres with a test that applies a create and then a rename suggestion for every one of the 12 entity types, against an index built before the applies. It then rebuilds the index from the database and compares each upserted entry with the rebuilt one (Type, ID, Title and Search text):

• master: 0 of 12 types found after the create;
• this PR: 12 of 12 found, every rename reflected, and every upserted entry identical to the rebuilt one;
• a task flipped private → public → private through update suggestions enters and leaves the index as handleUpdateTask does;
• the full internal/isms/api package passes with ISMS_TEST_DATABASE_URL set, and go vet and gofmt are clean.

The findings below are about keeping it that way. The new builders are not used by the HTTP handlers for the same five types (F1). The tests cover 3 of the 12 types and neither direction of the task privacy flip (F2). Findings are keyed F1, F2, … so they don't autolink to issues.

Out of scope, noted for a follow-up issue rather than this PR: the objective and program HTTP handlers (api_objectives.go) never touch the search index. After this PR, an objective or program created through a suggestion is searchable at once, but one created through the form waits for the next rebuild. (Checked by reading the source: api_objectives.go has no search index calls.)

Pre-existing, not caused by this PR, and the same for the HTTP handlers: when an apply lands while a background rebuild (started by a search on an index older than 5 minutes) is already reading the database, the rebuild's Set can overwrite the fresh upsert. The record then disappears again until the next rebuild. Also, Search loops over the org's entries slice after releasing the read lock, while Upsert replaces entries in place under the write lock.

---

F1 [should-fix] internal/isms/api/api_search.go line 269

The new builders (incidentSearchEntry, assetSearchEntry, taskSearchEntry, changeSearchEntry, correctiveActionSearchEntry) are used by buildSearchIndex and the apply handlers. The HTTP handlers for the same five types still spell the entry out inline through searchUpsert, at 13 call sites:

• api_collab.go 2414, 2704 (task create/update), 2951, 3152 (change create/update), 3320, 3349 (change follow-up task)
• api_corrective.go 145, 311
• api_incidents.go 172, 397, 652
• server.go 2081, 2813 (asset create/update)

So each of these types now has three spellings of its search entry instead of one: the builder, plus the inline string in each HTTP handler. The comment above the builders says the build and upsert paths "cannot drift", but that only holds between rebuild and apply. If someone later adds a field to incidentSearchEntry, POST /incidents keeps indexing the old text until the next rebuild. The five types that already had builders (risk, system, supplier, legal, audit finding) don't have this problem, because their HTTP handlers call the builder. #500's suggested shape asked for one builder per type shared with the HTTP handlers: "Use the same per-type entry builder as the HTTP handlers … so the two paths cannot drift. Types that still build their search text inline need a builder first".

Today the inline strings match the builders character for character, so search behaves the same. This is about maintenance, not a bug you can see now (checked by reading the source).

Suggested fix: the same mechanical swap the PR already made in buildSearchIndex. After it, searchUpsert has no callers left and can be deleted:

// api_collab.go handleCreateTask
s.searchIndex.Upsert(orgID, taskSearchEntry(&t))
// api_collab.go handleUpdateTask: t is a copy of *old, so t.Identifier == old.Identifier
s.searchIndex.Upsert(orgID, taskSearchEntry(&t))
// api_collab.go handleCreateChange / handleUpdateChange
s.searchIndex.Upsert(orgID, changeSearchEntry(&cr))
s.searchIndex.Upsert(orgID, changeSearchEntry(updated))
// api_collab.go syncChangeFollowupTask
s.searchIndex.Upsert(orgID, taskSearchEntry(existing))
s.searchIndex.Upsert(orgID, taskSearchEntry(t))
// api_corrective.go
s.searchIndex.Upsert(orgID, correctiveActionSearchEntry(&ca))
s.searchIndex.Upsert(orgID, correctiveActionSearchEntry(existing))
// api_incidents.go
s.searchIndex.Upsert(orgID, incidentSearchEntry(&inc))
s.searchIndex.Upsert(orgID, incidentSearchEntry(existing))
s.searchIndex.Upsert(orgID, incidentSearchEntry(after))
// server.go
s.searchIndex.Upsert(orgID, assetSearchEntry(&a))
s.searchIndex.Upsert(orgID, assetSearchEntry(after))

I applied exactly this on top of the PR head and deleted searchUpsert (13 lines changed, 10 removed). go build ./internal/..., go vet and gofmt are clean, and the full internal/isms/api package passes against Postgres.

---

F2 [should-fix] internal/isms/api/api_suggestions_search_pg_test.go line 92

The description says all twelve types are covered, and the applyTaskUpdate comment promises that "a rename or a private→public flip is searchable right away". The tests exercise risk create and rename, supplier create, and task create (public and private). Nine of the 12 create handlers, 10 of the 11 update handlers, and both task privacy flips have no test. If one of those registerApplyPostCommit lines is lost in a later refactor, CI stays green. No other test in the package searches after an apply, so nothing else would catch it.

Below are two tests that cover the rest. I ran them on both sides. On master, all 12 types fail ("no … entry with id") and so does the flip. On this PR, all pass. The parity check (the upserted entry must equal what a rebuild produces) is what actually holds the "cannot drift" claim in place. Append them to the PR's test file and add context, fmt and net/http to its imports:

func applySuggestionID(t *testing.T, s *Server, orgID int, sg *db.Suggestion) string {
	t.Helper()
	sg = newTestSuggestion(t, s, orgID, sg)
	c, rec := suggestionCtx(orgID, http.MethodPost, sg.ID)
	if err := s.handleApplyEntitySuggestion(c); err != nil {
		t.Fatalf("apply %s:%s: %v", sg.EntityType, sg.SuggestionType, err)
	}
	var out struct {
		Status string `json:"status"`
		ID     string `json:"applied_entity_id"`
	}
	if err := json.Unmarshal(rec.Body.Bytes(), &out); err != nil || out.Status != "applied" {
		t.Fatalf("apply %s:%s: %d %s", sg.EntityType, sg.SuggestionType, rec.Code, rec.Body.String())
	}
	return out.ID
}

func searchIndexEntry(s *Server, orgID int, typ, id string) (SearchEntry, bool) {
	s.searchIndex.mu.RLock()
	defer s.searchIndex.mu.RUnlock()
	for _, e := range s.searchIndex.entries[orgID] {
		if e.Type == typ && e.ID == id {
			return e, true
		}
	}
	return SearchEntry{}, false
}

func TestSuggestionApplySearchParityAllTypes(t *testing.T) {
	s := testServer(t)
	ctx := context.Background()
	orgID := newTestOrg(t, s, "suggestion-search-all")
	contractTestUser(t, s, orgID, suggestionChangelogActor, "manager")

	ap := &db.AuditProgramme{Title: "search programme", Year: 2026, Status: "draft", CreatedBy: suggestionChangelogActor}
	if err := s.db.CreateAuditProgramme(ctx, orgID, ap); err != nil {
		t.Fatalf("CreateAuditProgramme: %v", err)
	}
	audit := &db.Audit{ProgrammeID: &ap.ID, Title: "search audit", AuditType: "internal", Status: "planned", Auditor: suggestionChangelogActor}
	if err := s.db.CreateAudit(ctx, orgID, audit); err != nil {
		t.Fatalf("CreateAudit: %v", err)
	}

	s.buildSearchIndex(orgID)

	type tc struct {
		entity, typ, create, renameField string
	}
	cases := []tc{
		{"risk", "risk", `{"title":"Alpha risk","description":"alpha body","risk_type":"` + db.RiskTypes[0] + `","origin":"` + db.RiskOrigins[0] + `","status":"` + db.RiskStatuses[0] + `","treatment":"mitigate","current_likelihood":2,"current_impact":3}`, "title"},
		{"incident", "incident", `{"title":"Alpha incident","description":"alpha body"}`, "title"},
		{"supplier", "supplier", `{"name":"Alpha supplier","notes":"alpha body"}`, "name"},
		{"legal_requirement", "legal", `{"title":"Alpha legal","description":"alpha body"}`, "title"},
		{"change_request", "change", `{"title":"Alpha change","description":"alpha body"}`, "title"},
		{"corrective_action", "corrective_action", `{"title":"Alpha ca","description":"alpha body"}`, "title"},
		{"task", "task", `{"title":"Alpha task","description":"alpha body"}`, "title"},
		{"program", "program", `{"key":"srch","title":"Alpha program","description":"alpha body"}`, ""},
		{"objective", "objective", `{"title":"Alpha objective","description":"alpha body","program_key":"SRCH"}`, "title"},
		{"system", "system", `{"name":"Alpha system","description":"alpha body"}`, "name"},
		{"asset", "asset", `{"name":"Alpha asset","description":"alpha body"}`, "name"},
		{"audit_finding", "audit_finding", fmt.Sprintf(`{"audit_id":%d,"title":"Alpha finding","description":"alpha body"}`, audit.ID), "title"},
	}

	upserted := map[string]SearchEntry{}
	for _, c := range cases {
		t.Run(c.entity, func(t *testing.T) {
			id := applySuggestionID(t, s, orgID, &db.Suggestion{EntityType: c.entity, SuggestionType: "create", Payload: json.RawMessage(c.create)})
			e, ok := searchIndexEntry(s, orgID, c.typ, id)
			if !ok {
				t.Fatalf("after create: no %s entry with id %q", c.typ, id)
			}
			if c.renameField != "" {
				newName := "Omega " + strings.ReplaceAll(c.entity, "_", " ")
				applySuggestionID(t, s, orgID, &db.Suggestion{EntityType: c.entity, EntityID: id, SuggestionType: "update",
					Payload: json.RawMessage(fmt.Sprintf(`{"fields":{%q:%q}}`, c.renameField, newName))})
				e, ok = searchIndexEntry(s, orgID, c.typ, id)
				if !ok || e.Title != newName {
					t.Fatalf("after rename: entry %+v ok=%v, want title %q", e, ok, newName)
				}
			}
			upserted[c.typ+"|"+id] = e
		})
	}

	// Rebuild from the database and compare entry by entry.
	s.buildSearchIndex(orgID)
	for k, want := range upserted {
		typ, id, _ := strings.Cut(k, "|")
		got, ok := searchIndexEntry(s, orgID, typ, id)
		if !ok {
			t.Errorf("%s: missing after rebuild", k)
			continue
		}
		g := fmt.Sprintf("%s | %s | %s | %q", got.Type, got.ID, got.Title, got.Search)
		w := fmt.Sprintf("%s | %s | %s | %q", want.Type, want.ID, want.Title, want.Search)
		if g != w {
			t.Errorf("%s: rebuild and apply-upsert disagree\n  rebuild: %s\n  upsert:  %s", k, g, w)
		}
	}
}

// Task visibility flips through update suggestions: private -> public enters the
// index, public -> private leaves it, a rename while private stays out.
func TestSuggestionApplyTaskPrivacyFlip(t *testing.T) {
	s := testServer(t)
	orgID := newTestOrg(t, s, "suggestion-search-flip")
	contractTestUser(t, s, orgID, suggestionChangelogActor, "manager")
	s.buildSearchIndex(orgID)

	state := func(id string) string {
		e, ok := searchIndexEntry(s, orgID, "task", id)
		if !ok {
			return "absent"
		}
		return "present:" + e.Title
	}
	upd := func(id, fields string) {
		applySuggestionID(t, s, orgID, &db.Suggestion{EntityType: "task", EntityID: id, SuggestionType: "update", Payload: json.RawMessage(`{"fields":` + fields + `}`)})
	}
	id := applySuggestionID(t, s, orgID, &db.Suggestion{EntityType: "task", SuggestionType: "create", Payload: json.RawMessage(`{"title":"Secret start","private":true}`)})
	var got []string
	got = append(got, "created-private="+state(id))
	upd(id, `{"title":"Secret renamed"}`)
	got = append(got, "renamed-private="+state(id))
	upd(id, `{"private":false}`)
	got = append(got, "made-public="+state(id))
	upd(id, `{"title":"Public renamed"}`)
	got = append(got, "renamed-public="+state(id))
	upd(id, `{"private":true}`)
	got = append(got, "made-private="+state(id))
	want := "created-private=absent | renamed-private=absent | made-public=present:Secret renamed | renamed-public=present:Public renamed | made-private=absent"
	if g := strings.Join(got, " | "); g != want {
		t.Fatalf("\n got: %s\nwant: %s", g, want)
	}
}

Run with:

ISMS_TEST_DATABASE_URL="postgres://…/isms_db_test?sslmode=disable" go test ./internal/isms/api -run 'TestSuggestionApply' -count=1 -v

Caveats: the tests read the index directly, not through GET /search, so they say nothing about the HTTP layer or the frontend. They ran against a local Postgres 14 with the migrations applied, not the postgres:17 CI image. They also don't model a background rebuild running at the same moment as an apply. That race is described in the summary at the top; it predates this PR and affects the HTTP handlers the same way.

---

F3 [nit] internal/isms/api/api_search.go line 150, internal/isms/api/api_suggestions.go line 1568

Two comments read wrong after the change:

• The block // buildSearchIndex loads all entity types … objectiveSearchEntries / programSearchEntries build the index rows … now sits directly above func objectiveSearchEntry, so godoc attaches it to the single-item builder it doesn't describe. (The buildSearchIndex line was already stranded there before this PR.) Suggest moving the objectiveSearchEntries / programSearchEntries sentence above objectiveSearchEntries, and dropping the stray buildSearchIndex line or moving it to buildSearchIndex.
• In applyTaskUpdate: "a task that is (or becomes) private leaves it; a created or public one is upserted". This is an update handler, so nothing is "created" here. Probably meant "a public one (including one just made public) is upserted".

---

PR description verification

• "Fixes #500" and the root cause: the apply handlers never patched the index, and "the only search call there was the task removal for private tasks". Confirmed. On master (fc37e92) the only search call in api_suggestions.go is the private-task searchRemove. The PR's own two tests fail on master with "0 results" and pass on this PR.
• "post-commit via registerApplyPostCommit … so a rolled-back apply never reaches the index". Confirmed (by reading the source, no PoC). handleApplyEntitySuggestion drains postCommit only after WithOrgTx returns nil. WithOrgTx makes a single attempt with no retry, so a callback can't be registered twice. A handler that fails after registering, such as applyTaskUpdate failing in LogChangesTx, returns an error, and the queue is dropped.
• "every create/update apply handler now registers s.searchIndex.Upsert(orgID, <type>SearchEntry(&entity)), the same builder its HTTP counterpart uses". Partially confirmed. All 23 create/update handlers (11 types with both, plus program create) register the upsert. "The same builder its HTTP counterpart uses" is true for risk, system, supplier, legal and audit finding. For incident, asset, task, change and corrective action, the HTTP handlers still inline the entry (F1). Objectives and programs have no HTTP counterpart that touches the index at all.
• "buildSearchIndex now uses them … so the rebuild path and the upsert path cannot drift". Confirmed for rebuild vs apply. The parity test in F2 compares all 12 types and finds each upserted entry identical to the rebuilt one. It does not hold for rebuild vs the HTTP upserts of the five types in F1.
• "a public task is upserted, a private task never enters the shared index (#178), and a public→private flip still removes the entry". Confirmed, by the flip test in F2: created-private absent, renamed-private absent, made-public present, renamed-public present with the new title, made-private absent.
• "All twelve entity types with a create or update apply handler are covered". Confirmed. The applyRegistry has create or update handlers for exactly these 12 types (program has create only). Master: 0 of 12 found after apply. This PR: 12 of 12. As a test claim, only 3 of the 12 are tested (F2).
• "Readings, reassessments and review suggestions change nothing the index holds". Confirmed (by reading the source). applyRiskReading and applyLegalReading write entity_readings. The supplier, access and asset review handlers insert review rows and don't update the parent's name, title or description. applyIncidentLink only adds links.
• "Both fail on master … and pass with the fix … the same setup CI's 'Database round-trip tests' step uses". Confirmed. .github/workflows/tests.yml lines 139-140 run go test … ./internal/isms/api/ with ISMS_TEST_DATABASE_URL set, so the new tests run in CI. The integration and go-unit checks are green on c0f35c5.
• "go vet, gofmt -l . clean". Confirmed on the changed packages.
• "the full go test ./internal/isms/... run shows no new failures versus baseline (on Windows …)". Not verified on Windows. On macOS, the full internal/isms/api package passes against Postgres on this PR. I didn't run the other internal/isms/... packages.

[bfs2021]

All three applied in dc83473.

• F1: swapped all 13 HTTP handler call sites to incidentSearchEntry, assetSearchEntry, taskSearchEntry, changeSearchEntry and correctiveActionSearchEntry, and deleted searchUpsert — no callers left. Rebuild, apply and HTTP paths now share one builder per type.
• F2: appended both tests to api_suggestions_search_pg_test.go (TestSuggestionApplySearchParityAllTypes, TestSuggestionApplyTaskPrivacyFlip) with the context / fmt / net/http imports. They compile here, but this machine has no usable Postgres, so the CI database step will be their first real run — same setup as the existing tests in that file.
• F3: moved the objectiveSearchEntries / programSearchEntries sentence above objectiveSearchEntries, put the buildSearchIndex line back on its function, and reworded the applyTaskUpdate comment.

go build ./internal/..., go vet ./internal/..., gofmt -l and the non-database tests in internal/isms/api are clean. TestValidateUpdatePayload fails on the branch head before this commit too (pre-existing, unrelated).

[unidoc-alip]

Verdict: APPROVE — 0 must-fix, 0 should-fix, 0 nit

Re-review of dc83473, which addresses the first review of c0f35c5 (F1, F2, F3). Findings from that review are keyed F1, F2, … so they don't autolink to issues. All three are resolved and the new commit introduces nothing new.

What I checked, against a local Postgres with the migrations applied:

• The PR head (dc83473) and the PR merged into current master (baeb781, which now includes #308): the full internal/isms/api package passes with ISMS_TEST_DATABASE_URL set on both trees. That includes the four suggestion search tests and all 12 subtests of TestSuggestionApplySearchParityAllTypes. The merge is clean, and go build ./internal/..., go vet and gofmt -l are clean on the merged tree.
• You mentioned the CI database step would be the first real run of the two new tests. CI's integration job on dc83473 is green, and its "Database round-trip tests" step runs internal/isms/api with the database URL set. It runs without -v, so the log shows only the package ok. The local run above gives the per-test results.
• TestValidateUpdatePayload passes on macOS on both trees, which fits your reading that its failure is specific to Windows.

---

Resolution of the first review

• F1 (one builder per type): resolved. All 13 HTTP call sites now call s.searchIndex.Upsert(orgID, <type>SearchEntry(...)), and searchUpsert is gone (no callers left in internal/ or cmd/). The swap doesn't change any search text. Each deleted inline string matches its builder character for character. The one call that used a different variable, handleUpdateTask (old.Identifier, now t.Identifier), indexes the same ID, because t comes from prepareTaskUpdate as a copy of *old and taskUpdateRequest has no identifier field. So the 12 entity types now have one entry builder each, shared by rebuild, apply and HTTP. The only SearchEntry literal outside a builder is the document entry in buildSearchIndex, which is outside this PR.
• F2 (test coverage): resolved. TestSuggestionApplySearchParityAllTypes and TestSuggestionApplyTaskPrivacyFlip are added exactly as proposed. Both pass, as described above. On master they fail, as shown in the first review.
• F3 (comments): resolved. The buildSearchIndex comment is back on its function. The objectiveSearchEntries / programSearchEntries sentence now sits above objectiveSearchEntries, and objectiveSearchEntry has its own accurate comment. The applyTaskUpdate comment no longer mentions a "created" task.

---

PR description verification

Only the claims touched by dc83473 are re-checked here; the rest stand as verified in the first review.

• "every create/update apply handler now registers s.searchIndex.Upsert(orgID, <type>SearchEntry(&entity)), the same builder its HTTP counterpart uses": confirmed. In the first review this was only partly true, for 5 of the 10 types with HTTP upserts. Now it holds for all 10. Objectives and programs still have no HTTP handler that touches the index. That gap was outside this PR and noted for a follow-up in the first review.
• "buildSearchIndex now uses them … so the rebuild path and the upsert path cannot drift": confirmed for rebuild vs apply (the parity test compares all 12 types) and for rebuild vs HTTP, where there is now only one spelling per type.
• "All twelve entity types with a create or update apply handler are covered": confirmed, now as a test claim too. The parity test creates each of the 12 types through a suggestion, renames the 11 that have an update handler, and checks each entry against a rebuild.
• Author's comment, "go build ./internal/..., go vet ./internal/..., gofmt -l … are clean": confirmed on the merged tree for build, go vet ./internal/isms/api/ and gofmt -l internal/isms/api.