diff --git a/core/services/studio/capabilities.go b/core/services/studio/capabilities.go index 51d1164629..8890db9309 100644 --- a/core/services/studio/capabilities.go +++ b/core/services/studio/capabilities.go @@ -281,6 +281,8 @@ func (s *Service) CommandRunner(ctx context.Context, user, conv string) (Command // commandsFor says whether the chat of rec can run commands at its next turn. func (s *Service) commandsFor(ctx context.Context, p string, rec session.ChatRecord) (Commands, error) { conv := rec.ID + ctx, closeScope := s.executionScope(ctx, p, conv) + defer closeScope() a := s.chatActorFor(ctx, rec) if rec.Kind == session.KindSubAgent && rec.AgentID == "" { s.mu.Lock() @@ -538,6 +540,8 @@ func (s *Service) addonCommandDescriptorForActor(ctx context.Context, a workspac if sc != nil && sc.deleted { return Commands{State: CommandsNone}, nil } + ctx, closeScope := s.executionScope(ctx, p, rec.ID) + defer closeScope() var tools []session.Tool // Only ad-hoc children inherit a pruned tool set. Saved-agent children // resolve current grants through their scope, just like their turn tools. diff --git a/core/services/studio/documenteligibility_internal_test.go b/core/services/studio/documenteligibility_internal_test.go index 4f61cdaac9..b90ea48b68 100644 --- a/core/services/studio/documenteligibility_internal_test.go +++ b/core/services/studio/documenteligibility_internal_test.go @@ -121,6 +121,8 @@ var _ = Describe("document provider matrix", func() { } b.sets = append(b.sets, set) } + ctx, closeScope := svc.executionScope(ctx, "alice", "") + defer closeScope() list, _, _, err := svc.chatSkillSet(ctx, "alice", nil, off, sc, command) Expect(err).NotTo(HaveOccurred()) usable := skills.Usable(list) @@ -401,6 +403,8 @@ func documentCommands(s *Service, ctx context.Context, p string, rec session.Cha // chatSkillNotes resolves the skills of a chat as the turn does and returns // the notes it gets. func chatSkillNotes(svc *Service, ctx context.Context, off []string, command Commands) ([]skills.Skill, []string, error) { + ctx, closeScope := svc.executionScope(ctx, "alice", "") + defer closeScope() list, notes, _, err := svc.chatSkillSet(ctx, "alice", nil, off, nil, command) return list, notes, err } diff --git a/core/services/studio/executionobservation.go b/core/services/studio/executionobservation.go index a022d48738..38670f9f4d 100644 --- a/core/services/studio/executionobservation.go +++ b/core/services/studio/executionobservation.go @@ -4,6 +4,8 @@ package studio import ( "cmp" "context" + "log/slog" + "runtime/debug" "sync" "time" @@ -101,12 +103,28 @@ func ExecutionObservation(ctx context.Context, user, addon string) (model.Value, return value, detail.Explanation } +var scopelessOnce sync.Once + +// onScopeless is called when a status is read from a context that has no +// execution scope. That is a programming error: the read cannot succeed and +// the caller would report "not selected" for an add-on that is fine. It logs +// once; the tests replace it to fail. +var onScopeless = func() { + scopelessOnce.Do(func() { + slog.Error("studio: the execution status was read outside an execution scope; the status is reported as not selected", "stack", string(debug.Stack())) + }) +} + // ObserveExecution is ExecutionObservation with the whole detail. Failure says // why the value is unknown; Explanation and NetworkPolicyEnforced are set only // when the provider answered. func ObserveExecution(ctx context.Context, user, addon string) (model.Value, ExecutionDetail) { scope, ok := ctx.Value(executionObservationKey{}).(*executionObservation) - if !ok || user != scope.user { + if !ok { + onScopeless() + return model.Unknown(), ExecutionDetail{Failure: UnknownNotSelected} + } + if user != scope.user { return model.Unknown(), ExecutionDetail{Failure: UnknownNotSelected} } scope.mu.Lock() diff --git a/core/services/studio/loadskill.go b/core/services/studio/loadskill.go index 44830a489d..9a2bdfb605 100644 --- a/core/services/studio/loadskill.go +++ b/core/services/studio/loadskill.go @@ -134,6 +134,10 @@ func (s *Service) loadableSkillsForActor(ctx context.Context, a workspace.Actor, } func (s *Service) loadableSkillsAndNotesForActor(ctx context.Context, a workspace.Actor, final *bool) ([]skills.Skill, []string, error) { + // The harness calls a tool with a context of its own, without the turn's + // scope. The status of the commands is read here, so the scope is too. + ctx, closeScope := s.executionScope(ctx, a.Principal, a.ChatID) + defer closeScope() rec, err := s.d.Store.GetChat(ctx, a.Principal, a.ChatID) if err != nil { return nil, nil, err diff --git a/core/services/studio/scopeguard_internal_test.go b/core/services/studio/scopeguard_internal_test.go new file mode 100644 index 0000000000..f713e46d88 --- /dev/null +++ b/core/services/studio/scopeguard_internal_test.go @@ -0,0 +1,27 @@ +package studio + +import ( + "runtime/debug" + "sync/atomic" + + . "github.com/onsi/ginkgo/v2" +) + +// A status read without an execution scope is a programming error. Every +// spec fails when one happens, with the stack of the first. +var scopeless atomic.Pointer[string] + +var _ = BeforeSuite(func() { + onScopeless = func() { + stack := string(debug.Stack()) + scopeless.CompareAndSwap(nil, &stack) + } +}) + +var _ = BeforeEach(func() { scopeless.Store(nil) }) + +var _ = AfterEach(func() { + if stack := scopeless.Load(); stack != nil { + Fail("the execution status was read outside an execution scope:\n" + *stack) + } +})