Sync merge validates config but does not reliably apply live role/workflow/provider definitions

[michaelw]

Summary

Incoming config from MergeConfiguration is validated/normalized, but new and updated role/workflow/provider definitions do not reliably become the live in-memory config.

This appears to originate in 38739ebd (making progress on config diffs). The intent in that commit and later follow-ups (9f84556, 709f9738) appears to be to apply incoming synced config immediately, but the live config does not reflect those changes.

Why this looks like a bug rather than intended behavior

• RegisterWithThandServer calls MergeConfiguration(registration) and then returns; there is no later persistence step that would justify discarding the normalized result.
• The naming and comments in the sync path read as though incoming config should be applied, not merely validated.
• git log upstream/main..origin/mw/main -- internal/config/sync.go is empty, so this does not appear to come from fork-only integration work.

Minimal repro

I copied focused regression tests into an untouched upstream/main worktree and ran:

go test ./internal/config -run 'TestMergeConfiguration_(ServerSends(New|Updated)(Roles|Workflow|Provider)s?|PartialConfig_OnlyRoles)|TestApplyPatch_AppliesRoles'

Focused failing tests:

• TestMergeConfiguration_ServerSendsNewRoles
• TestMergeConfiguration_ServerSendsNewWorkflows
• TestMergeConfiguration_ServerSendsUpdatedWorkflow
• TestMergeConfiguration_ServerSendsNewProviders
• TestMergeConfiguration_ServerSendsUpdatedProvider
• TestMergeConfiguration_PartialConfig_OnlyRoles
• TestApplyPatch_AppliesRoles

Representative failing output:

--- FAIL: TestMergeConfiguration_ServerSendsNewRoles (0.00s)
    sync_test.go:172:
        Messages:    expected synced role to be stored locally
--- FAIL: TestMergeConfiguration_ServerSendsNewWorkflows (0.00s)
    sync_test.go:230:
        Messages:    expected synced workflow to be stored locally
--- FAIL: TestMergeConfiguration_ServerSendsUpdatedWorkflow (0.07s)
    sync_test.go:264:
        expected: "Updated workflow"
        actual  : "Existing workflow"
--- FAIL: TestMergeConfiguration_ServerSendsNewProviders (0.00s)
    sync_test.go:288:
        Messages:    expected synced provider to be stored locally
--- FAIL: TestMergeConfiguration_ServerSendsUpdatedProvider (0.06s)
    sync_test.go:322:
        expected: "Updated provider description"
        actual  : "Old provider description"
--- FAIL: TestApplyPatch_AppliesRoles (0.00s)
    sync_test.go:618:
        Error:       map[string]models.Role(nil) does not contain "new-role"

Analysis

The likely failure mode is that the sync path computes merged/normalized definitions but does not reliably commit them back onto the live config object, and updated entries can also be reduced to ineffective partial typed state before apply.

Scope

This issue is only about the sync path failing to apply incoming config into live in-memory definitions.

Please keep separate from:

• stale-snapshot/lost-update concurrency in the sync path
• broader lock-discipline cleanup across config readers/writers

[michaelw]

Exact test source used for the failing evidence run on untouched upstream/main (from the local apply-fix commit test file):

func makeTestWorkflow(name, description string) models.Workflow {
	return models.Workflow{
		Name:        name,
		Description: description,
		Enabled:     true,
		Workflow: &model.Workflow{
			Do: &model.TaskList{},
		},
	}
}

func makeTestProvider(name, description string) models.ProviderConfig {
	return models.ProviderConfig{
		Name:        name,
		Description: description,
		Provider:    "mock",
		Enabled:     true,
	}
}

// waitForPatch waits for a single PATCH call on the channel, or times out.
func waitForPatch(ch <-chan syncPatchCall, timeout time.Duration) (syncPatchCall, bool) {
	select {
	case call := <-ch:
		return call, true
	case <-time.After(timeout):
		return syncPatchCall{}, false
	}
}

// ---------------------------------------------------------------------------
// Tests for MergeConfiguration — incoming config merging
// ---------------------------------------------------------------------------

func TestMergeConfiguration_ServerSendsNewRoles(t *testing.T) {
	server, patchCh := newSyncTestServer(t)

	// Local config has no roles
	config := newSyncTestConfig(t, nil, nil, nil, server.URL)

	// Server sends a new role
	reg := makeRegistrationResponse(
		map[string]models.Role{
			"admin": {Name: "admin", Enabled: true},
		},
		nil, nil,
	)

	err := config.MergeConfiguration(reg)
	require.NoError(t, err)

	role, exists := config.Roles.Definitions["admin"]
	require.True(t, exists, "expected synced role to be stored locally")
	assert.Equal(t, "admin", role.Name)
	assert.True(t, role.Enabled)

	_, ok := waitForPatch(patchCh, 5*time.Second)
	require.True(t, ok, "expected outgoing PATCH call")
}

func TestMergeConfiguration_ServerSendsUpdatedRole(t *testing.T) {
	server, patchCh := newSyncTestServer(t)

	// Local config has an existing role
	config := newSyncTestConfig(t,
		map[string]models.Role{
			"editor":    {Name: "editor", Description: "Can edit", Enabled: true},
			"untouched": {Name: "untouched", Description: "Keep me", Enabled: true},
		},
		nil, nil, server.URL,
	)

	// Server sends the same role with updated description
	reg := makeRegistrationResponse(
		map[string]models.Role{
			"editor": {Name: "editor", Description: "Can edit and publish", Enabled: true},
		},
		nil, nil,
	)

	err := config.MergeConfiguration(reg)
	require.NoError(t, err)

	role, exists := config.Roles.Definitions["editor"]
	require.True(t, exists, "expected synced role to be stored locally")
	assert.Equal(t, "Can edit and publish", role.Description)
	assert.True(t, role.Enabled)
	assert.Contains(t, config.Roles.Definitions, "untouched")

	_, ok := waitForPatch(patchCh, 5*time.Second)
	require.True(t, ok, "expected outgoing PATCH call")
}

func TestMergeConfiguration_ServerSendsNewWorkflows(t *testing.T) {
	server, patchCh := newSyncTestServer(t)

	config := newSyncTestConfig(t, nil, nil, nil, server.URL)

	reg := makeRegistrationResponse(
		nil,
		map[string]models.Workflow{
			"approval": makeTestWorkflow("approval", "Handles approvals"),
		},
		nil,
	)

	err := config.MergeConfiguration(reg)
	require.NoError(t, err)

	workflow, exists := config.Workflows.Definitions["approval"]
	require.True(t, exists, "expected synced workflow to be stored locally")
	assert.Equal(t, "Handles approvals", workflow.Description)
	require.NotNil(t, workflow.Workflow)

	_, ok := waitForPatch(patchCh, 5*time.Second)
	require.True(t, ok, "expected outgoing PATCH call")
}

func TestMergeConfiguration_ServerSendsUpdatedWorkflow(t *testing.T) {
	server, patchCh := newSyncTestServer(t)

	config := newSyncTestConfig(t,
		nil,
		map[string]models.Workflow{
			"existing":  makeTestWorkflow("existing", "Existing workflow"),
			"unchanged": makeTestWorkflow("unchanged", "Keep me"),
		},
		nil,
		server.URL,
	)

	reg := makeRegistrationResponse(
		nil,
		map[string]models.Workflow{
			"existing": makeTestWorkflow("existing", "Updated workflow"),
		},
		nil,
	)

	err := config.MergeConfiguration(reg)
	require.NoError(t, err)

	workflow, exists := config.Workflows.Definitions["existing"]
	require.True(t, exists, "expected synced workflow to be stored locally")
	assert.Equal(t, "Updated workflow", workflow.Description)
	assert.Contains(t, config.Workflows.Definitions, "unchanged")

	_, ok := waitForPatch(patchCh, 5*time.Second)
	require.True(t, ok, "expected outgoing PATCH call")
}

func TestMergeConfiguration_ServerSendsNewProviders(t *testing.T) {
	server, patchCh := newSyncTestServer(t)

	config := newSyncTestConfig(t, nil, nil, nil, server.URL)

	reg := makeRegistrationResponse(
		nil,
		nil,
		map[string]models.ProviderConfig{
			"mock-primary": makeTestProvider("mock-primary", "Primary mock provider"),
		},
	)

	err := config.MergeConfiguration(reg)
	require.NoError(t, err)

	provider, exists := config.Providers.Definitions["mock-primary"]
	require.True(t, exists, "expected synced provider to be stored locally")
	assert.Equal(t, "Primary mock provider", provider.Description)
	assert.Equal(t, "mock", provider.Provider)

	_, ok := waitForPatch(patchCh, 5*time.Second)
	require.True(t, ok, "expected outgoing PATCH call")
}

func TestMergeConfiguration_ServerSendsUpdatedProvider(t *testing.T) {
	server, patchCh := newSyncTestServer(t)

	config := newSyncTestConfig(t,
		nil,
		nil,
		map[string]models.ProviderConfig{
			"mock-primary": makeTestProvider("mock-primary", "Old provider description"),
			"mock-extra":   makeTestProvider("mock-extra", "Keep me"),
		},
		server.URL,
	)

	reg := makeRegistrationResponse(
		nil,
		nil,
		map[string]models.ProviderConfig{
			"mock-primary": makeTestProvider("mock-primary", "Updated provider description"),
		},
	)

	err := config.MergeConfiguration(reg)
	require.NoError(t, err)

	provider, exists := config.Providers.Definitions["mock-primary"]
	require.True(t, exists, "expected synced provider to be stored locally")
	assert.Equal(t, "Updated provider description", provider.Description)
	assert.Contains(t, config.Providers.Definitions, "mock-extra")

	_, ok := waitForPatch(patchCh, 5*time.Second)
	require.True(t, ok, "expected outgoing PATCH call")
}

func TestMergeConfiguration_IdenticalConfigs(t *testing.T) {
	server, patchCh := newSyncTestServer(t)

	roles := map[string]models.Role{
		"viewer": {Name: "viewer", Description: "Read-only", Enabled: true},
	}

	config := newSyncTestConfig(t, roles, nil, nil, server.URL)

	// Server sends the exact same roles
	reg := makeRegistrationResponse(roles, nil, nil)

	err := config.MergeConfiguration(reg)
	require.NoError(t, err)

	// The outgoing goroutine still fires (with an empty or no-op patch)
	_, ok := waitForPatch(patchCh, 5*time.Second)
	require.True(t, ok, "expected outgoing PATCH call")
}

func TestMergeConfiguration_PartialConfig_OnlyRoles(t *testing.T) {
	server, patchCh := newSyncTestServer(t)

	config := newSyncTestConfig(t,
		map[string]models.Role{
			"existing": {Name: "existing", Enabled: true},
		},
		nil, nil, server.URL,
	)

	// Server sends only new roles, no workflows or providers
	reg := makeRegistrationResponse(
		map[string]models.Role{
			"new-role": {Name: "new-role", Enabled: true},
		},
		nil, nil,
	)

	err := config.MergeConfiguration(reg)
	require.NoError(t, err)

	role, exists := config.Roles.Definitions["new-role"]
	require.True(t, exists, "expected synced role to be stored locally")
	assert.Equal(t, "new-role", role.Name)
	assert.Contains(t, config.Roles.Definitions, "existing")

	_, ok := waitForPatch(patchCh, 5*time.Second)
	require.True(t, ok, "expected outgoing PATCH call")
}

func TestApplyPatch_AppliesRoles(t *testing.T) {
	config := newSyncTestConfig(t, nil, nil, nil, "")

	diff := ConfigPatchRequest{
		RoleConfig: &RoleConfig{
			Definitions: map[string]models.Role{
				"new-role": {Name: "new-role", Enabled: true},
			},
		},
	}

	err := config.applyPatch(diff)
	assert.NoError(t, err)
	assert.Contains(t, config.Roles.Definitions, "new-role")
}