Treat config definition entries as immutable snapshots and eliminate legacy in-place mutations

[michaelw]

Summary

Config sync retry logic now assumes configuration definition entries behave like immutable snapshots, but the codebase still has legacy mutation-prone paths that can mutate nested state in place.

Desired invariant

Configuration definition maps should be treated as immutable snapshots. When config changes, callers should replace whole entries or whole definition maps rather than mutating nested state in place.

Existing mutation-prone examples

• Provider config resolution mutates nested BasicConfig through ProviderConfig.ResolveConfig.
• Role accessors and resolution paths can work on shallow copies whose slice-backed fields still alias underlying data.
• Workflow definitions contain pointer-backed *model.Workflow state, so shallow copies do not provide isolation.

Follow-up work

• Audit and replace existing mutation-prone paths with copy-on-write or whole-entry replacement.
• Standardize the invariant in config code and docs.
• Add targeted tests for mutation isolation beyond the sync retry path.

Scope note

This is a follow-up enhancement issue. It should stay separate from the current sync bug-fix PRs so reviewers can reason about behavior fixes independently from broader lock/mutation cleanup.

[michaelw]

Follow-up note from the sync retry fix:

• The current branch now keeps sync retry/commit logic definitions-only and avoids full-map JSON equality under c.mu.Lock.
• It also adds skipped evidence tests in internal/config/sync_test.go documenting the remaining mutation-safety gap for nested provider, role, and workflow mutations.
• Those tests are intentionally skipped until the broader mutation audit lands, because the current config subsystem still allows nested in-place mutations that can bypass configGeneration.

Concrete next steps for #306:

1. Audit the current mutation sites (ProviderConfig.ResolveConfig, role permission mutation helpers, workflow nested pointer mutation paths).
2. Decide whether to enforce immutability by copy-on-write accessors or by standardizing whole-entry replacement semantics.
3. Turn the skipped evidence tests into real passing regression tests once the mutation cleanup lands.

[michaelw]

Evidence update for the skipped mutation-gap tests.

This was added only in an isolated temp worktree at upstream/main (b8af027) to demonstrate that the underlying nested-mutation problem predated the sync retry/generation work. I added equivalent direct-mutability repros in a separate temp worktree and ran the same focused command. Those also fail:

--- FAIL: TestKnownGap_NestedProviderMutationLeaksIntoStoredConfig (0.00s)
    sync_test.go:290:
        Error:       Not equal:
                     expected: "us-east-1"
                     actual  : "eu-west-1"
        Messages:    nested provider mutation should not leak into stored config
--- FAIL: TestKnownGap_NestedRoleMutationLeaksIntoStoredConfig (0.00s)
    sync_test.go:314:
        Error:       Not equal:
                     expected: "s3:GetObject"
                     actual  : "storage.objects.get"
        Messages:    nested role mutation should not leak into stored config
--- FAIL: TestKnownGap_NestedWorkflowMutationLeaksIntoStoredConfig (0.00s)
    sync_test.go:331:
        Error:       Expected value not to be nil.
        Messages:    nested workflow mutation should not leak into stored config
FAIL

Interpretation:

• The underlying nested-mutation/aliasing problem is pre-existing and already present on upstream/main.
• What is specific to the current branch is the observable failure mode via configGeneration: once sync retry/CAS logic exists, these pre-existing in-place mutations become visible as mutations that bypass generation tracking.
• So the skipped tests on this branch are documenting a real residual gap, but not a newly introduced root cause.

func TestKnownGap_NestedProviderMutationLeaksIntoStoredConfig(t *testing.T) {
	config := &Config{
		Providers: ProviderDefinitionsConfig{
			Definitions: map[string]models.ProviderConfig{
				"mock-primary": makeTestProviderWithConfig("mock-primary", "primary", map[string]any{
					"region": "us-east-1",
				}),
			},
		},
	}

	provider := config.Providers.Definitions["mock-primary"]
	require.NotNil(t, provider.Config)
	provider.Config.SetKeyWithValue("region", "eu-west-1")

	storedProvider := config.Providers.Definitions["mock-primary"]
	require.NotNil(t, storedProvider.Config)
	region, ok := storedProvider.Config.GetString("region")
	require.True(t, ok)
	assert.Equal(t, "us-east-1", region, "nested provider mutation should not leak into stored config")
}

func TestKnownGap_NestedRoleMutationLeaksIntoStoredConfig(t *testing.T) {
	config := newSyncTestConfig(t,
		map[string]models.Role{
			"editor": {
				Name:      "editor",
				Providers: []string{"aws-prod"},
				Permissions: models.RolePermissions{
					Allow: models.RoleStatements{{
						Operations: []string{"s3:GetObject"},
					}},
				},
				Enabled: true,
			},
		},
		nil, nil, "",
	)

	role := config.Roles.Definitions["editor"]
	role.Permissions.Allow[0].Operations[0] = "storage.objects.get"

	storedRole := config.Roles.Definitions["editor"]
	assert.Equal(t, "s3:GetObject", storedRole.Permissions.Allow[0].Operations[0], "nested role mutation should not leak into stored config")
}

func TestKnownGap_NestedWorkflowMutationLeaksIntoStoredConfig(t *testing.T) {
	config := newSyncTestConfig(t,
		nil,
		map[string]models.Workflow{
			"approval": makeTestWorkflow("approval", "original"),
		},
		nil, "",
	)

	workflow := config.Workflows.Definitions["approval"]
	workflow.Workflow.Do = nil

	storedWorkflow := config.Workflows.Definitions["approval"]
	require.NotNil(t, storedWorkflow.Workflow)
	assert.NotNil(t, storedWorkflow.Workflow.Do, "nested workflow mutation should not leak into stored config")
}

[michaelw]

Nuance on the upstream repros versus real upstream code paths:

• I haven’t found a strong normal upstream path that mutates the returned workflow definition in place.
• The upstream tests prove a real mutability property of the current config structs: nested provider/role/workflow state is aliased and can leak back into stored config.
• But they are not all equally strong as demonstrations of a currently exercised upstream runtime race.

What looks plausibly real in normal upstream code paths today:

• Roles: this is the strongest case. RoleConfig.GetRoleByName returns a shallow copy (internal/config/roles.go), and normal role-resolution paths call mutating helpers like filterRoleByProviders, mergeRolePermissions, resolvePermissionConflicts, and ExpandWildcardPermissionsForProvider. Because the copied Role still carries slice-backed nested state, those mutations can plausibly leak back into stored definitions during ordinary role/composite-role resolution, including while sync or other reads are happening.

What is currently more of a mutability proof than a proven runtime race:

• Providers: the direct repro is real, but the most obvious mutation site (initializeSingleProvider -> ResolveConfig) works on a copied provider value inside InitializeProviders, so that path does not by itself prove a write-back into stored provider definitions.
• Workflows: GetWorkflowByName returns a shallow copy containing *model.Workflow, so aliasing is real, but I have not yet identified a strong upstream call path that normally mutates the returned workflow definition in place.

So the right reading is:

• the root mutability/aliasing problem is pre-existing upstream;
• the role case appears plausibly reachable in normal upstream behavior;
• the provider/workflow repros currently serve more as evidence that the data structures are unsafe-by-construction unless callers maintain an immutability discipline.