Skills: allow fork skills to override the model

docs/features/skills/index.md

@@ -98,6 +98,7 @@ When asked to create a Dockerfile:
 | `name`           | Yes      | Unique skill identifier                                                     |
 | `description`    | Yes      | Short description shown to the agent for skill matching                     |
 | `context`        | No       | Set to `fork` to run the skill as an isolated sub-agent (see below)         |
+| `model`          | No       | Override the model used while running the skill as a sub-agent (fork only)  |
 | `allowed-tools`  | No       | List of tools the skill needs (YAML list or comma-separated string)         |
 | `license`        | No       | License identifier (e.g. `Apache-2.0`)                                      |
 | `compatibility`  | No       | Free-text compatibility notes                                               |
@@ -138,6 +139,45 @@ When the agent encounters a task that matches a `context: fork` skill, it uses t
 
 </div>
 
+### Overriding the model for a fork skill
+
+Fork skills can declare a `model` field in their frontmatter to use a
+different model than the parent agent for the duration of the sub-session.
+This is useful when a skill is best handled by a faster, cheaper, or more
+specialised model — for example a powerful reasoning model for refactors,
+or a fast model for routine bookkeeping work. The override only applies
+while the skill is running; the parent agent keeps its own model.
+
+The `model` value accepts either a named model from the agent config or
+an inline `provider/model` reference (and the same comma-separated alloy
+syntax as the rest of the agent config):
+
+<!-- yaml-lint:skip -->
+```yaml
+---
+name: bump-go-dependencies
+description: Update Go module dependencies one by one
+context: fork
+model: openai/gpt-4o-mini
+---
+
+# Bump Dependencies
+
+1. ...
+```
+
+If the model reference cannot be resolved (unknown name, missing
+credentials, runtime not configured for model switching, …) the skill
+falls back to the agent's currently-active model (its configured
+default, or any override the user previously set via the model picker)
+and a warning is logged.
+
+When the skill completes, the agent's previous model is restored — but
+only if no one else changed the model in the meantime. If the user
+switches the model via the TUI model picker while the fork skill is
+running, their choice is preserved (the deferred restore becomes a
+no-op).
+
 ## Search Paths
 
 Skills are discovered from these locations (later overrides earlier):

pkg/agent/agent.go

@@ -152,7 +152,13 @@ func (a *Agent) Model() provider.Provider {
 // The override(s) take precedence over the configured models.
 // For alloy models, multiple providers can be passed and one will be randomly selected.
 // Pass no arguments or nil providers to clear the override.
-func (a *Agent) SetModelOverride(models ...provider.Provider) {
+//
+// SetModelOverride returns a snapshot of the value that was just stored.
+// Callers performing a scoped override (apply now, restore later) should
+// keep this snapshot and pass it as `current` to RestoreModelOverride so
+// the deferred restore can detect concurrent changes via CAS. Callers
+// that only need the side-effect can ignore the return value.
+func (a *Agent) SetModelOverride(models ...provider.Provider) ModelOverrideSnapshot {
 	// Filter out nil providers
 	var validModels []provider.Provider
 	for _, m := range models {
@@ -161,17 +167,20 @@ func (a *Agent) SetModelOverride(models ...provider.Provider) {
 		}
 	}
 
+	var ptr *[]provider.Provider
 	if len(validModels) == 0 {
 		a.modelOverrides.Store(nil)
 		slog.Debug("Cleared model override", "agent", a.name)
 	} else {
-		a.modelOverrides.Store(&validModels)
+		ptr = &validModels
+		a.modelOverrides.Store(ptr)
 		ids := make([]string, len(validModels))
 		for i, m := range validModels {
 			ids[i] = m.ID()
 		}
 		slog.Debug("Set model override", "agent", a.name, "models", ids)
 	}
+	return ModelOverrideSnapshot{ptr: ptr}
 }
 
 // HasModelOverride returns true if a model override is currently set.
@@ -180,6 +189,39 @@ func (a *Agent) HasModelOverride() bool {
 	return overrides != nil && len(*overrides) > 0
 }
 
+// ModelOverrideSnapshot is an opaque token that captures the agent's model
+// override at a point in time. Pass it to RestoreModelOverride to undo a
+// scoped override safely.
+type ModelOverrideSnapshot struct {
+	// ptr is the raw atomic pointer value at snapshot time. It is used for
+	// pointer-identity compare-and-swap, never dereferenced by callers.
+	ptr *[]provider.Provider
+}
+
+// SnapshotModelOverride captures the agent's current model override. The
+// returned snapshot is opaque; pass it to RestoreModelOverride later to
+// restore the captured value.
+func (a *Agent) SnapshotModelOverride() ModelOverrideSnapshot {
+	return ModelOverrideSnapshot{ptr: a.modelOverrides.Load()}
+}
+
+// RestoreModelOverride atomically restores the override to the value
+// captured by `prev`, but only if the current override is still the one
+// captured by `current` (pointer identity). If another caller has changed
+// the override since `current` was captured, the restore is a no-op so
+// that the concurrent change wins.
+//
+// This is the safe primitive for applying a temporary override around a
+// scope (e.g. a skill sub-session) without clobbering changes made by
+// concurrent callers such as the TUI model picker.
+func (a *Agent) RestoreModelOverride(prev, current ModelOverrideSnapshot) {
+	if a.modelOverrides.CompareAndSwap(current.ptr, prev.ptr) {
+		slog.Debug("Restored model override", "agent", a.name)
+	} else {
+		slog.Debug("Model override changed concurrently; skipping restore", "agent", a.name)
+	}
+}
+
 // ConfiguredModels returns the originally configured models for this agent.
 // This is useful for listing available models in the TUI picker.
 func (a *Agent) ConfiguredModels() []provider.Provider {

pkg/agent/agent_test.go

@@ -145,30 +145,147 @@ func TestModelOverride(t *testing.T) {
 	a := New("root", "test", WithModel(defaultModel))
 
 	// Initially should return the default model
-	model := a.Model()
-	assert.Equal(t, "openai/gpt-4o", model.ID())
+	assert.Equal(t, "openai/gpt-4o", a.Model().ID())
 	assert.False(t, a.HasModelOverride())
 
 	// Set an override
 	a.SetModelOverride(overrideModel)
 	assert.True(t, a.HasModelOverride())
+	assert.Equal(t, "anthropic/claude-sonnet-4-0", a.Model().ID())
 
-	// Now Model() should return the override
-	model = a.Model()
-	assert.Equal(t, "anthropic/claude-sonnet-4-0", model.ID())
-
-	// ConfiguredModels should still return the original models
+	// ConfiguredModels still reflects the originally configured models
 	configuredModels := a.ConfiguredModels()
 	require.Len(t, configuredModels, 1)
 	assert.Equal(t, "openai/gpt-4o", configuredModels[0].ID())
 
 	// Clear the override
 	a.SetModelOverride(nil)
 	assert.False(t, a.HasModelOverride())
+	assert.Equal(t, "openai/gpt-4o", a.Model().ID())
+}
+
+func TestSetModelOverride_ReturnsSnapshotOfStoredValue(t *testing.T) {
+	// SetModelOverride must return a snapshot of the value it just stored,
+	// not what a subsequent SnapshotModelOverride() would load. This is the
+	// guarantee that closes the race window for scoped overrides: if a
+	// concurrent caller stores a different override after our store but
+	// before we capture our snapshot, our snapshot must still refer to
+	// what we stored, so the deferred CAS-restore will fail (concurrent
+	// change wins) instead of incorrectly succeeding.
+	t.Parallel()
+
+	defaultModel := &mockProvider{id: "default"}
+	oursModel := &mockProvider{id: "ours"}
+	othersModel := &mockProvider{id: "others"}
+
+	a := New("root", "test", WithModel(defaultModel))
+
+	// Capture the snapshot returned by SetModelOverride.
+	prev := a.SnapshotModelOverride()
+	oursSnap := a.SetModelOverride(oursModel)
+
+	// Simulate a concurrent caller storing a different override _after_ we
+	// stored ours but _before_ a hypothetical post-store SnapshotModelOverride.
+	a.SetModelOverride(othersModel)
+	require.Equal(t, "others", a.Model().ID())
+
+	// The deferred restore must be a no-op because oursSnap holds the
+	// pointer we stored, not the current pointer.
+	a.RestoreModelOverride(prev, oursSnap)
+	assert.Equal(t, "others", a.Model().ID(),
+		"concurrent override must be preserved; the snapshot returned by SetModelOverride captures the stored pointer")
+}
 
-	// Model() should return the default again
-	model = a.Model()
-	assert.Equal(t, "openai/gpt-4o", model.ID())
+func TestSetModelOverride_ClearReturnsZeroSnapshot(t *testing.T) {
+	t.Parallel()
+
+	a := New("root", "test", WithModel(&mockProvider{id: "default"}))
+
+	// Calling SetModelOverride with no providers (or nil) clears the override.
+	// The returned snapshot should round-trip cleanly through RestoreModelOverride.
+	cleared := a.SetModelOverride()
+	assert.False(t, a.HasModelOverride())
+
+	// Now set an override and restore using `cleared` as `prev`.
+	oursSnap := a.SetModelOverride(&mockProvider{id: "ours"})
+	require.True(t, a.HasModelOverride())
+
+	a.RestoreModelOverride(cleared, oursSnap)
+	assert.False(t, a.HasModelOverride(), "restoring a cleared snapshot must clear the override")
+}
+
+func TestSnapshotAndRestoreModelOverride(t *testing.T) {
+	t.Parallel()
+
+	defaultModel := &mockProvider{id: "openai/gpt-4o"}
+	skillModel := &mockProvider{id: "openai/gpt-4o-mini"}
+	userModel := &mockProvider{id: "anthropic/claude-sonnet-4-0"}
+
+	t.Run("restores when no concurrent change", func(t *testing.T) {
+		t.Parallel()
+		a := New("root", "test", WithModel(defaultModel))
+
+		prev := a.SnapshotModelOverride()
+		a.SetModelOverride(skillModel)
+		ours := a.SnapshotModelOverride()
+		assert.Equal(t, "openai/gpt-4o-mini", a.Model().ID())
+
+		a.RestoreModelOverride(prev, ours)
+		assert.False(t, a.HasModelOverride())
+		assert.Equal(t, "openai/gpt-4o", a.Model().ID())
+	})
+
+	t.Run("restores back to a pre-existing override", func(t *testing.T) {
+		t.Parallel()
+		a := New("root", "test", WithModel(defaultModel))
+		a.SetModelOverride(userModel)
+
+		prev := a.SnapshotModelOverride()
+		a.SetModelOverride(skillModel)
+		ours := a.SnapshotModelOverride()
+		assert.Equal(t, "openai/gpt-4o-mini", a.Model().ID())
+
+		a.RestoreModelOverride(prev, ours)
+		assert.Equal(t, "anthropic/claude-sonnet-4-0", a.Model().ID())
+	})
+
+	t.Run("keeps a concurrent change instead of restoring", func(t *testing.T) {
+		// This is the TUI-while-skill-runs scenario: another caller
+		// changes the override between SnapshotModelOverride and
+		// RestoreModelOverride. The deferred restore must NOT clobber
+		// that change.
+		t.Parallel()
+		a := New("root", "test", WithModel(defaultModel))
+
+		prev := a.SnapshotModelOverride()
+		a.SetModelOverride(skillModel)
+		ours := a.SnapshotModelOverride()
+
+		// Simulate concurrent TUI model switch.
+		a.SetModelOverride(userModel)
+
+		a.RestoreModelOverride(prev, ours)
+		require.True(t, a.HasModelOverride(), "user's model choice must be preserved")
+		assert.Equal(t, "anthropic/claude-sonnet-4-0", a.Model().ID())
+	})
+
+	t.Run("keeps a concurrent clear instead of restoring", func(t *testing.T) {
+		// Same as above but the concurrent caller clears the override
+		// (e.g. user revert via TUI). The restore must respect that.
+		t.Parallel()
+		a := New("root", "test", WithModel(defaultModel))
+		a.SetModelOverride(userModel)
+
+		prev := a.SnapshotModelOverride()
+		a.SetModelOverride(skillModel)
+		ours := a.SnapshotModelOverride()
+
+		// Simulate concurrent TUI revert.
+		a.SetModelOverride()
+
+		a.RestoreModelOverride(prev, ours)
+		assert.False(t, a.HasModelOverride(), "user's revert must be preserved")
+	})
 }
 
 func TestModel_LogsSelection(t *testing.T) {