diff --git a/cmd/harnessd/embedded_cron_test.go b/cmd/harnessd/embedded_cron_test.go index e25dc97a..e7e53db7 100644 --- a/cmd/harnessd/embedded_cron_test.go +++ b/cmd/harnessd/embedded_cron_test.go @@ -2,7 +2,10 @@ package main import ( "context" + "encoding/json" + "errors" "fmt" + "net/http/httptest" "path/filepath" "reflect" "strings" @@ -15,6 +18,7 @@ import ( "go-agent-harness/internal/fakeprovider" "go-agent-harness/internal/harness" htools "go-agent-harness/internal/harness/tools" + "go-agent-harness/internal/harness/tools/deferred" ) func newTestEmbeddedAdapter(t *testing.T) *embeddedCronAdapter { @@ -41,6 +45,530 @@ func newTestEmbeddedAdapter(t *testing.T) *embeddedCronAdapter { return &embeddedCronAdapter{store: st, scheduler: sched, clock: clock} } +type injectedEmbeddedCronScheduler struct { + delegate *cron.Scheduler + addErr error + updateErr error + beforeAddFail func() + beforeUpdateFail func() +} + +func (s *injectedEmbeddedCronScheduler) AddJob(job cron.Job) error { + if s.addErr != nil { + if s.beforeAddFail != nil { + s.beforeAddFail() + } + return s.addErr + } + return s.delegate.AddJob(job) +} + +func (s *injectedEmbeddedCronScheduler) PrepareJob(job cron.Job) (*cron.PreparedJob, error) { + if s.delegate.HasEntry(job.ID) && s.updateErr != nil { + if s.beforeUpdateFail != nil { + s.beforeUpdateFail() + } + return nil, s.updateErr + } + if s.addErr != nil { + if s.beforeAddFail != nil { + s.beforeAddFail() + } + return nil, s.addErr + } + return s.delegate.PrepareJob(job) +} + +func (s *injectedEmbeddedCronScheduler) CommitJob(prepared *cron.PreparedJob) { + s.delegate.CommitJob(prepared) +} + +func (s *injectedEmbeddedCronScheduler) AbortJob(prepared *cron.PreparedJob) { + s.delegate.AbortJob(prepared) +} + +func (s *injectedEmbeddedCronScheduler) UpdateJobSchedule(job cron.Job) error { + if s.updateErr != nil { + if s.beforeUpdateFail != nil { + s.beforeUpdateFail() + } + return s.updateErr + } + return s.delegate.UpdateJobSchedule(job) +} + +func (s *injectedEmbeddedCronScheduler) RemoveJob(id string) { s.delegate.RemoveJob(id) } +func (s *injectedEmbeddedCronScheduler) HasEntry(id string) bool { + return s.delegate.HasEntry(id) +} + +type deleteFailingEmbeddedCronStore struct { + cron.Store + err error +} + +func (s *deleteFailingEmbeddedCronStore) DeleteJob(context.Context, string) error { return s.err } +func (s *deleteFailingEmbeddedCronStore) DeactivateJob(context.Context, string) error { return s.err } + +func TestEmbeddedCronCreate_SchedulerAndDeleteFailureLeavesDurablyInactiveJob(t *testing.T) { + baseStore := newTestCronStore(t) + store := &deleteFailingEmbeddedCronStore{Store: baseStore, err: errors.New("injected delete failure")} + clock := testClock{t: time.Date(2026, 3, 8, 12, 0, 0, 0, time.UTC)} + realScheduler := cron.NewScheduler(store, &cron.ShellExecutor{}, clock, cron.SchedulerConfig{MaxConcurrent: 1}) + t.Cleanup(realScheduler.Stop) + scheduler := &injectedEmbeddedCronScheduler{delegate: realScheduler, addErr: errors.New("injected add failure")} + adapter := &embeddedCronAdapter{store: store, scheduler: scheduler, clock: clock} + + _, err := adapter.CreateJob(context.Background(), htools.CronCreateJobRequest{ + Name: "create-fail-closed", Schedule: "*/5 * * * *", ExecType: cron.ExecTypeShell, ExecConfig: `{"command":"echo ok"}`, + }) + if err == nil { + t.Fatal("CreateJob succeeded despite scheduler failure") + } + jobs, listErr := baseStore.ListJobs(context.Background()) + if listErr != nil { + t.Fatalf("list jobs: %v", listErr) + } + if len(jobs) != 1 { + t.Fatalf("jobs = %d, want one retained fail-closed record", len(jobs)) + } + if jobs[0].Status != cron.StatusPaused { + t.Fatalf("retained status = %q, want paused", jobs[0].Status) + } + if scheduler.HasEntry(jobs[0].ID) { + t.Fatal("failed create left a runnable scheduler entry") + } +} + +type activationFailingEmbeddedCronStore struct{ cron.Store } + +func (activationFailingEmbeddedCronStore) UpdateJobCAS(context.Context, cron.Job, time.Time) error { + return errors.New("injected activation failure") +} + +type updateCASFailingEmbeddedCronStore struct{ cron.Store } + +func (updateCASFailingEmbeddedCronStore) UpdateJobCAS(context.Context, cron.Job, time.Time) error { + return errors.New("injected update CAS failure") +} + +func TestEmbeddedCronUpdate_PreparedReplacementCASFailureAbortsAndPreservesOldJob(t *testing.T) { + store := newTestCronStore(t) + clock := testClock{t: time.Date(2026, 3, 8, 12, 0, 0, 0, time.UTC)} + scheduler := cron.NewScheduler(store, &cron.ShellExecutor{}, clock, cron.SchedulerConfig{MaxConcurrent: 1}) + t.Cleanup(scheduler.Stop) + adapter := &embeddedCronAdapter{store: store, scheduler: scheduler, clock: clock} + created, err := adapter.CreateJob(context.Background(), htools.CronCreateJobRequest{Name: "prepared-cas-failure", Schedule: "*/5 * * * *", ExecType: cron.ExecTypeShell, ExecConfig: `{"command":"echo ok"}`}) + if err != nil { + t.Fatalf("create: %v", err) + } + adapter.store = updateCASFailingEmbeddedCronStore{Store: store} + newSchedule := "0 * * * *" + if _, err := adapter.UpdateJob(context.Background(), created.ID, htools.CronUpdateJobRequest{Schedule: &newSchedule, ExpectedUpdatedAt: &created.UpdatedAt}); err == nil { + t.Fatal("schedule update succeeded despite CAS failure") + } + persisted, err := store.GetJob(context.Background(), created.ID) + if err != nil { + t.Fatalf("load persisted: %v", err) + } + if persisted.Schedule != created.Schedule || persisted.Status != created.Status || persisted.ExecConfig != created.ExecConfig || !persisted.UpdatedAt.Equal(created.UpdatedAt) { + t.Fatalf("persisted after CAS failure = %#v, want unchanged created %#v", persisted, created) + } + if !scheduler.HasEntry(created.ID) { + t.Fatal("CAS failure removed the old live scheduler entry") + } +} + +func TestEmbeddedCronUpdate_RedundantActiveStatusDoesNotReregister(t *testing.T) { + store := newTestCronStore(t) + clock := testClock{t: time.Date(2026, 3, 8, 12, 0, 0, 0, time.UTC)} + realScheduler := cron.NewScheduler(store, &cron.ShellExecutor{}, clock, cron.SchedulerConfig{MaxConcurrent: 1}) + t.Cleanup(realScheduler.Stop) + adapter := &embeddedCronAdapter{store: store, scheduler: realScheduler, clock: clock} + created, err := adapter.CreateJob(context.Background(), htools.CronCreateJobRequest{Name: "redundant-active", Schedule: "*/5 * * * *", ExecType: cron.ExecTypeShell, ExecConfig: `{"command":"echo ok"}`}) + if err != nil { + t.Fatalf("create: %v", err) + } + injected := &injectedEmbeddedCronScheduler{delegate: realScheduler, addErr: errors.New("unexpected add")} + adapter.scheduler = injected + active, tags := cron.StatusActive, "changed" + updated, err := adapter.UpdateJob(context.Background(), created.ID, htools.CronUpdateJobRequest{Status: &active, Tags: &tags, ExpectedUpdatedAt: &created.UpdatedAt}) + if err != nil { + t.Fatalf("redundant active update: %v", err) + } + if updated.Tags != tags || updated.Status != cron.StatusActive || !realScheduler.HasEntry(created.ID) { + t.Fatalf("redundant active result = %#v, entry=%v", updated, realScheduler.HasEntry(created.ID)) + } +} + +func TestEmbeddedCronCreate_ActivationFailureNeverRestartRearmsPausedJob(t *testing.T) { + baseStore := newTestCronStore(t) + clock := testClock{t: time.Date(2026, 3, 8, 12, 0, 0, 0, time.UTC)} + store := activationFailingEmbeddedCronStore{Store: baseStore} + scheduler := cron.NewScheduler(store, &cron.ShellExecutor{}, clock, cron.SchedulerConfig{MaxConcurrent: 1}) + t.Cleanup(scheduler.Stop) + adapter := &embeddedCronAdapter{store: store, scheduler: scheduler, clock: clock} + if _, err := adapter.CreateJob(context.Background(), htools.CronCreateJobRequest{Name: "activation-fail", Schedule: "*/5 * * * *", ExecType: cron.ExecTypeShell, ExecConfig: `{"command":"echo ok"}`}); err == nil { + t.Fatal("CreateJob succeeded despite activation failure") + } + jobs, err := baseStore.ListJobs(context.Background()) + if err != nil || len(jobs) != 1 || jobs[0].Status != cron.StatusPaused { + t.Fatalf("durable create after activation failure = %#v, %v", jobs, err) + } + if scheduler.HasEntry(jobs[0].ID) { + t.Fatal("activation failure left a live entry") + } + restarted := cron.NewScheduler(baseStore, &cron.ShellExecutor{}, clock, cron.SchedulerConfig{MaxConcurrent: 1}) + t.Cleanup(restarted.Stop) + if err := restarted.Start(context.Background()); err != nil { + t.Fatalf("restart: %v", err) + } + if restarted.HasEntry(jobs[0].ID) { + t.Fatal("paused failed create rearmed after restart") + } +} + +func TestEmbeddedCronUpdate_AddFailureAndRollbackConflictConvergesFailClosed(t *testing.T) { + store := newTestCronStore(t) + clock := testClock{t: time.Date(2026, 3, 8, 12, 0, 0, 0, time.UTC)} + realScheduler := cron.NewScheduler(store, &cron.ShellExecutor{}, clock, cron.SchedulerConfig{MaxConcurrent: 1}) + t.Cleanup(realScheduler.Stop) + scheduler := &injectedEmbeddedCronScheduler{delegate: realScheduler} + adapter := &embeddedCronAdapter{store: store, scheduler: scheduler, clock: clock} + + created, err := adapter.CreateJob(context.Background(), htools.CronCreateJobRequest{ + Name: "resume-fail-closed", Schedule: "*/5 * * * *", ExecType: cron.ExecTypeShell, ExecConfig: `{"command":"echo ok"}`, + }) + if err != nil { + t.Fatalf("create: %v", err) + } + pausedStatus := cron.StatusPaused + paused, err := adapter.UpdateJob(context.Background(), created.ID, htools.CronUpdateJobRequest{ + Status: &pausedStatus, ExpectedUpdatedAt: &created.UpdatedAt, + }) + if err != nil { + t.Fatalf("pause: %v", err) + } + if scheduler.HasEntry(created.ID) { + t.Fatal("pause left a scheduler entry") + } + + var touchOnce sync.Once + scheduler.beforeAddFail = func() { + touchOnce.Do(func() { + current, getErr := store.GetJob(context.Background(), created.ID) + if getErr != nil { + t.Fatalf("load before concurrent touch: %v", getErr) + } + if touchErr := store.TouchJobRun( + context.Background(), current.ID, clock.Now(), current.NextRunAt, current.UpdatedAt.Add(time.Second), + ); touchErr != nil { + t.Fatalf("concurrent touch: %v", touchErr) + } + }) + } + scheduler.addErr = errors.New("injected persistent add failure") + scheduler.updateErr = errors.New("injected persistent replacement failure") + activeStatus := cron.StatusActive + if _, err := adapter.UpdateJob(context.Background(), created.ID, htools.CronUpdateJobRequest{ + Status: &activeStatus, ExpectedUpdatedAt: &paused.UpdatedAt, + }); err == nil { + t.Fatal("resume succeeded despite scheduler failure") + } + persisted, err := store.GetJob(context.Background(), created.ID) + if err != nil { + t.Fatalf("load after failed resume: %v", err) + } + if persisted.Status != cron.StatusPaused { + t.Fatalf("persisted status = %q, want fail-closed paused", persisted.Status) + } + if persisted.LastRunAt.IsZero() { + t.Fatal("fail-closed convergence overwrote the concurrent TouchJobRun") + } + if scheduler.HasEntry(created.ID) { + t.Fatal("fail-closed convergence left a runnable scheduler entry") + } +} + +func TestEmbeddedCronUpdate_ReplacementFailureAndRollbackConflictConvergesFailClosed(t *testing.T) { + store := newTestCronStore(t) + clock := testClock{t: time.Date(2026, 3, 8, 12, 0, 0, 0, time.UTC)} + realScheduler := cron.NewScheduler(store, &cron.ShellExecutor{}, clock, cron.SchedulerConfig{MaxConcurrent: 1}) + t.Cleanup(realScheduler.Stop) + scheduler := &injectedEmbeddedCronScheduler{delegate: realScheduler} + adapter := &embeddedCronAdapter{store: store, scheduler: scheduler, clock: clock} + + created, err := adapter.CreateJob(context.Background(), htools.CronCreateJobRequest{ + Name: "replacement-fail-closed", Schedule: "*/5 * * * *", ExecType: cron.ExecTypeShell, ExecConfig: `{"command":"echo ok"}`, + }) + if err != nil { + t.Fatalf("create: %v", err) + } + var touchOnce sync.Once + scheduler.beforeUpdateFail = func() { + touchOnce.Do(func() { + current, getErr := store.GetJob(context.Background(), created.ID) + if getErr != nil { + t.Fatalf("load before concurrent touch: %v", getErr) + } + if touchErr := store.TouchJobRun( + context.Background(), current.ID, clock.Now(), current.NextRunAt, current.UpdatedAt.Add(time.Second), + ); touchErr != nil { + t.Fatalf("concurrent touch: %v", touchErr) + } + }) + } + scheduler.updateErr = errors.New("injected persistent replacement failure") + newSchedule := "0 * * * *" + if _, err := adapter.UpdateJob(context.Background(), created.ID, htools.CronUpdateJobRequest{ + Schedule: &newSchedule, ExpectedUpdatedAt: &created.UpdatedAt, + }); err == nil { + t.Fatal("schedule update succeeded despite scheduler failure") + } + persisted, err := store.GetJob(context.Background(), created.ID) + if err != nil { + t.Fatalf("load after failed replacement: %v", err) + } + if persisted.ID != created.ID || persisted.Schedule != created.Schedule || persisted.Status != created.Status || persisted.ExecConfig != created.ExecConfig { + t.Fatalf("persisted after failed replacement = %#v, want pre-update config %#v", persisted, created) + } + if persisted.LastRunAt.IsZero() { + t.Fatal("fail-closed convergence overwrote the concurrent TouchJobRun") + } + if !scheduler.HasEntry(created.ID) { + t.Fatal("failed replacement removed the old runnable scheduler entry") + } +} + +func cronToolScope(tenant, conversation, agent string) context.Context { + return context.WithValue(context.Background(), htools.ContextKeyRunMetadata, htools.RunMetadata{ + TenantID: tenant, ConversationID: conversation, AgentID: agent, + }) +} + +func decodeCronToolJob(t *testing.T, result string) htools.CronJob { + t.Helper() + var job htools.CronJob + if err := json.Unmarshal([]byte(result), &job); err != nil { + t.Fatalf("decode cron job: %v (%s)", err, result) + } + return job +} + +func decodeCronGetToolJob(t *testing.T, result string) htools.CronJob { + t.Helper() + var payload struct { + Job htools.CronJob `json:"job"` + } + if err := json.Unmarshal([]byte(result), &payload); err != nil { + t.Fatalf("decode cron_get result: %v (%s)", err, result) + } + return payload.Job +} + +func TestEmbeddedCronModelToolsFullScopedLifecycle(t *testing.T) { + adapter := newTestEmbeddedAdapter(t) + client := deferred.NewScopedCronClient(adapter) + if err := client.Health(cronToolScope("tenant-a", "conversation-a", "agent-a")); err != nil { + t.Fatalf("scoped cron health: %v", err) + } + create := deferred.CronCreateTool(client) + list := deferred.CronListTool(client) + get := deferred.CronGetTool(client) + update := deferred.CronUpdateTool(client) + pause := deferred.CronPauseTool(client) + resume := deferred.CronResumeTool(client) + delete := deferred.CronDeleteTool(client) + + ctxA := cronToolScope("tenant-a", "conversation-a", "agent-a") + ctxB := cronToolScope("tenant-b", "conversation-b", "agent-b") + createdA := decodeCronToolJob(t, mustToolCall(t, create, ctxA, `{"name":"shared-name","schedule":"0 0 * * *","command":"echo initial","timeout_seconds":30}`)) + createdB := decodeCronToolJob(t, mustToolCall(t, create, ctxB, `{"name":"shared-name","schedule":"0 0 * * *","command":"echo other"}`)) + if createdA.ID == createdB.ID { + t.Fatal("scoped jobs must have distinct stable identities") + } + if _, err := get.Handler(ctxA, json.RawMessage(`{"id":"shared-name"}`)); err == nil { + t.Fatal("model-facing cron_get must accept job IDs only, never names") + } + + listedA := mustToolCall(t, list, ctxA, `{}`) + if !strings.Contains(listedA, createdA.ID) || strings.Contains(listedA, createdB.ID) { + t.Fatalf("scope A list leaked another conversation: %s", listedA) + } + if _, err := get.Handler(ctxB, json.RawMessage(fmt.Sprintf(`{"id":%q}`, createdA.ID))); err == nil { + t.Fatal("scope B must not read scope A's job") + } + if _, err := update.Handler(ctxB, json.RawMessage(fmt.Sprintf(`{"id":%q,"tags":"stolen","expected_updated_at":%q}`, createdA.ID, createdA.UpdatedAt.Format(time.RFC3339Nano)))); err == nil { + t.Fatal("scope B must not update scope A's job") + } + if _, err := pause.Handler(ctxB, json.RawMessage(fmt.Sprintf(`{"id":%q,"expected_updated_at":%q}`, createdA.ID, createdA.UpdatedAt.Format(time.RFC3339Nano)))); err == nil { + t.Fatal("scope B must not pause scope A's job") + } + if _, err := delete.Handler(ctxB, json.RawMessage(fmt.Sprintf(`{"id":%q,"expected_updated_at":%q}`, createdA.ID, createdA.UpdatedAt.Format(time.RFC3339Nano)))); err == nil { + t.Fatal("scope B must not delete scope A's job") + } + + gotA := decodeCronGetToolJob(t, mustToolCall(t, get, ctxA, fmt.Sprintf(`{"id":%q}`, createdA.ID))) + updated := decodeCronToolJob(t, mustToolCall(t, update, ctxA, fmt.Sprintf(`{"id":%q,"schedule":"15 * * * *","command":"echo updated","timeout_seconds":45,"tags":"updated","tenant_id":"spoofed","expected_updated_at":%q}`, createdA.ID, gotA.UpdatedAt.Format(time.RFC3339Nano)))) + if updated.ID != createdA.ID || updated.TenantID != "tenant-a" || updated.ConversationID != "conversation-a" || updated.AgentID != "agent-a" { + t.Fatalf("update changed identity or scope: %+v", updated) + } + if updated.Schedule != "15 * * * *" || updated.ExecConfig != `{"command":"echo updated"}` || updated.TimeoutSec != 45 || updated.Tags != "updated" { + t.Fatalf("updated values not applied: %+v", updated) + } + if _, err := update.Handler(ctxA, json.RawMessage(fmt.Sprintf(`{"id":%q,"timeout_seconds":0,"expected_updated_at":%q}`, createdA.ID, updated.UpdatedAt.Format(time.RFC3339Nano)))); err == nil { + t.Fatal("unsafe timeout must fail through the model-facing tool path") + } + + paused := decodeCronToolJob(t, mustToolCall(t, pause, ctxA, fmt.Sprintf(`{"id":%q,"expected_updated_at":%q}`, createdA.ID, updated.UpdatedAt.Format(time.RFC3339Nano)))) + if paused.Status != cron.StatusPaused || adapter.scheduler.HasEntry(createdA.ID) { + t.Fatalf("pause state = %q, scheduler entry = %v", paused.Status, adapter.scheduler.HasEntry(createdA.ID)) + } + resumed := decodeCronToolJob(t, mustToolCall(t, resume, ctxA, fmt.Sprintf(`{"id":%q,"expected_updated_at":%q}`, createdA.ID, paused.UpdatedAt.Format(time.RFC3339Nano)))) + if resumed.Status != cron.StatusActive || !adapter.scheduler.HasEntry(createdA.ID) { + t.Fatalf("resume state = %q, scheduler entry = %v", resumed.Status, adapter.scheduler.HasEntry(createdA.ID)) + } + + if _, err := get.Handler(ctxB, json.RawMessage(fmt.Sprintf(`{"id":%q}`, createdA.ID))); err == nil { + t.Fatal("scope B must not mutate or inspect scope A's job") + } + if _, err := delete.Handler(ctxA, json.RawMessage(fmt.Sprintf(`{"id":%q,"expected_updated_at":%q}`, createdA.ID, resumed.UpdatedAt.Format(time.RFC3339Nano)))); err != nil { + t.Fatalf("delete: %v", err) + } + if adapter.scheduler.HasEntry(createdA.ID) { + t.Fatal("delete must remove the scheduler entry") + } + if _, err := get.Handler(ctxA, json.RawMessage(fmt.Sprintf(`{"id":%q}`, createdA.ID))); err == nil { + t.Fatal("deleted job must be not found") + } + if strings.Contains(mustToolCall(t, list, ctxA, `{}`), createdA.ID) { + t.Fatal("deleted job remained in scoped list") + } +} + +func TestEmbeddedCronAdapterUsesAuthoritativeStoreScopePredicates(t *testing.T) { + adapter := newTestEmbeddedAdapter(t) + create := func(tenant string) htools.CronJob { + job, err := adapter.CreateJob(context.Background(), htools.CronCreateJobRequest{TenantID: tenant, ConversationID: "conversation", AgentID: "agent", Name: "same-name", Schedule: "0 0 * * *", ExecType: cron.ExecTypeShell, ExecConfig: `{"command":"echo ok"}`}) + if err != nil { + t.Fatalf("create %s: %v", tenant, err) + } + return job + } + jobA, jobB := create("tenant-a"), create("tenant-b") + if _, err := adapter.GetJobByName(context.Background(), "same-name"); !cron.IsJobAmbiguous(err) { + t.Fatalf("global operator name lookup = %v, want ambiguity", err) + } + ctxA := cron.WithScope(context.Background(), cron.Scope{TenantID: "tenant-a", ConversationID: "conversation", AgentID: "agent"}) + jobs, err := adapter.ListJobs(ctxA) + if err != nil || len(jobs) != 1 || jobs[0].ID != jobA.ID { + t.Fatalf("scoped list = %#v, %v", jobs, err) + } + if _, err := adapter.GetJob(ctxA, jobB.ID); !errors.Is(err, htools.ErrCronJobNotFound) { + t.Fatalf("cross-scope get = %v, want not found", err) + } + if _, err := adapter.UpdateJob(ctxA, jobB.ID, htools.CronUpdateJobRequest{}); !errors.Is(err, htools.ErrCronJobNotFound) { + t.Fatalf("cross-scope update = %v, want not found", err) + } + if _, err := adapter.ListExecutions(ctxA, jobB.ID, 10, 0); !errors.Is(err, htools.ErrCronJobNotFound) { + t.Fatalf("cross-scope history = %v, want not found", err) + } + if err := adapter.DeleteJob(ctxA, jobB.ID); !errors.Is(err, htools.ErrCronJobNotFound) { + t.Fatalf("cross-scope delete = %v, want not found", err) + } +} + +func mustToolCall(t *testing.T, tool htools.Tool, ctx context.Context, args string) string { + t.Helper() + result, err := tool.Handler(ctx, json.RawMessage(args)) + if err != nil { + t.Fatalf("%s: %v", tool.Definition.Name, err) + } + return result +} + +func assertDefaultRegistryCronScopeAndVersions(t *testing.T, rawClient htools.CronClient) { + t.Helper() + registry := harness.NewDefaultRegistryWithOptions(t.TempDir(), harness.DefaultRegistryOptions{ + ApprovalMode: harness.ToolApprovalModeFullAuto, + CronClient: rawClient, + }) + t.Cleanup(func() { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := registry.Shutdown(ctx); err != nil { + t.Logf("registry shutdown: %v", err) + } + }) + + ctxA := cronToolScope("tenant-a", "conversation-a", "agent-a") + ctxB := cronToolScope("tenant-b", "conversation-b", "agent-b") + createdResult, err := registry.Execute(ctxA, "cron_create", json.RawMessage(`{"name":"registry-scoped","schedule":"0 0 * * *","command":"echo initial"}`)) + if err != nil { + t.Fatalf("registry cron_create: %v", err) + } + created := decodeCronToolJob(t, createdResult) + + // Operator/server paths keep the raw adapter and therefore remain able to + // inspect jobs without model RunMetadata. + if got, err := rawClient.GetJob(context.Background(), created.ID); err != nil || got.ID != created.ID { + t.Fatalf("raw operator get = %#v, %v", got, err) + } + if _, err := registry.Execute(ctxB, "cron_get", json.RawMessage(fmt.Sprintf(`{"id":%q}`, created.ID))); !errors.Is(err, htools.ErrCronJobNotFound) { + t.Fatalf("cross-scope registry get = %v, want not found", err) + } + if _, err := registry.Execute(context.Background(), "cron_list", json.RawMessage(`{}`)); err == nil || !strings.Contains(err.Error(), "cron scope is required") { + t.Fatalf("unscoped registry list = %v, want required scope", err) + } + + updatedResult, err := registry.Execute(ctxA, "cron_update", json.RawMessage(fmt.Sprintf(`{"id":%q,"tags":"current","expected_updated_at":%q}`, created.ID, created.UpdatedAt.Format(time.RFC3339Nano)))) + if err != nil { + t.Fatalf("registry cron_update: %v", err) + } + updated := decodeCronToolJob(t, updatedResult) + if _, err := registry.Execute(ctxA, "cron_pause", json.RawMessage(fmt.Sprintf(`{"id":%q,"expected_updated_at":%q}`, created.ID, created.UpdatedAt.Format(time.RFC3339Nano)))); !errors.Is(err, htools.ErrCronJobConflict) { + t.Fatalf("stale registry pause = %v, want conflict", err) + } + pausedResult, err := registry.Execute(ctxA, "cron_pause", json.RawMessage(fmt.Sprintf(`{"id":%q,"expected_updated_at":%q}`, created.ID, updated.UpdatedAt.Format(time.RFC3339Nano)))) + if err != nil { + t.Fatalf("current registry pause: %v", err) + } + paused := decodeCronToolJob(t, pausedResult) + if _, err := registry.Execute(ctxA, "cron_resume", json.RawMessage(fmt.Sprintf(`{"id":%q,"expected_updated_at":%q}`, created.ID, updated.UpdatedAt.Format(time.RFC3339Nano)))); !errors.Is(err, htools.ErrCronJobConflict) { + t.Fatalf("stale registry resume = %v, want conflict", err) + } + resumedResult, err := registry.Execute(ctxA, "cron_resume", json.RawMessage(fmt.Sprintf(`{"id":%q,"expected_updated_at":%q}`, created.ID, paused.UpdatedAt.Format(time.RFC3339Nano)))) + if err != nil { + t.Fatalf("current registry resume: %v", err) + } + resumed := decodeCronToolJob(t, resumedResult) + if resumed.Status != cron.StatusActive { + t.Fatalf("resumed status = %q, want active", resumed.Status) + } + if _, err := registry.Execute(ctxA, "cron_delete", json.RawMessage(fmt.Sprintf(`{"id":%q,"expected_updated_at":%q}`, created.ID, paused.UpdatedAt.Format(time.RFC3339Nano)))); !errors.Is(err, htools.ErrCronJobConflict) { + t.Fatalf("stale registry delete = %v, want conflict", err) + } + if _, err := registry.Execute(ctxA, "cron_delete", json.RawMessage(fmt.Sprintf(`{"id":%q,"expected_updated_at":%q}`, created.ID, resumed.UpdatedAt.Format(time.RFC3339Nano)))); err != nil { + t.Fatalf("current registry delete: %v", err) + } +} + +func TestDefaultModelRegistryScopesEmbeddedAndRemoteCronAdapters(t *testing.T) { + t.Run("embedded", func(t *testing.T) { + assertDefaultRegistryCronScopeAndVersions(t, newTestEmbeddedAdapter(t)) + }) + t.Run("remote", func(t *testing.T) { + adapter := newTestEmbeddedAdapter(t) + scheduler, ok := adapter.scheduler.(*cron.Scheduler) + if !ok { + t.Fatalf("test adapter scheduler = %T, want *cron.Scheduler", adapter.scheduler) + } + server := httptest.NewServer(cron.NewServer(adapter.store, scheduler, adapter.clock)) + t.Cleanup(server.Close) + assertDefaultRegistryCronScopeAndVersions(t, &cronClientAdapter{client: cron.NewClient(server.URL)}) + }) +} + func TestEmbeddedCron_ScopedHarnessJobContinuesOwnedConversation(t *testing.T) { provider := fakeprovider.New( []fakeprovider.Turn{{Content: "scheduled reply"}}, @@ -88,18 +616,30 @@ func TestEmbeddedCron_ScopedHarnessJobContinuesOwnedConversation(t *testing.T) { } }) - job, err := bootstrap.client.CreateJob(context.Background(), htools.CronCreateJobRequest{ - Name: "continue-owned-conversation", - Schedule: "0 0 * * *", - ExecType: string(cron.ExecTypeHarness), - ExecConfig: `{"prompt":"scheduled follow-up"}`, - TenantID: "tenant-a", - ConversationID: "conversation-a", - AgentID: "agent-a", + scopedClient := deferred.NewScopedCronClient(bootstrap.client) + createTool := deferred.CronCreateTool(scopedClient) + getTool := deferred.CronGetTool(scopedClient) + updateTool := deferred.CronUpdateTool(scopedClient) + createCtx := context.WithValue(context.Background(), htools.ContextKeyRunMetadata, htools.RunMetadata{ + TenantID: "tenant-a", ConversationID: "conversation-a", AgentID: "agent-a", }) + createdResult, err := createTool.Handler(createCtx, json.RawMessage(`{"name":"continue-owned-conversation","schedule":"0 0 * * *","execution_type":"harness","prompt":"scheduled follow-up"}`)) if err != nil { - t.Fatalf("create cron job: %v", err) + t.Fatalf("create harness cron job through tool: %v", err) } + var job htools.CronJob + if err := json.Unmarshal([]byte(createdResult), &job); err != nil { + t.Fatalf("decode created cron job: %v", err) + } + if job.ExecType != string(cron.ExecTypeHarness) || job.ConversationID != "conversation-a" { + t.Fatalf("created harness job = %+v", job) + } + current := decodeCronGetToolJob(t, mustToolCall(t, getTool, createCtx, fmt.Sprintf(`{"id":%q}`, job.ID))) + job = decodeCronToolJob(t, mustToolCall(t, updateTool, createCtx, fmt.Sprintf( + `{"id":%q,"prompt":"updated scheduled follow-up","expected_updated_at":%q}`, + job.ID, + current.UpdatedAt.Format(time.RFC3339Nano), + ))) if err := bootstrap.scheduler.TriggerJob(context.Background(), job.ID); err != nil { t.Fatalf("trigger cron job: %v", err) @@ -134,8 +674,8 @@ func TestEmbeddedCron_ScopedHarnessJobContinuesOwnedConversation(t *testing.T) { final.AgentID, ) } - if final.Prompt != "scheduled follow-up" { - t.Fatalf("scheduled run prompt = %q, want %q", final.Prompt, "scheduled follow-up") + if final.Prompt != "updated scheduled follow-up" { + t.Fatalf("scheduled run prompt = %q, want %q", final.Prompt, "updated scheduled follow-up") } } @@ -148,7 +688,7 @@ func TestEmbeddedCronAdapter_CreateJob(t *testing.T) { Name: "test-job", Schedule: "*/5 * * * *", ExecType: "shell", - ExecConfig: `{"cmd":"echo hi"}`, + ExecConfig: `{"command":"echo hi"}`, TimeoutSec: 60, Tags: "test", TenantID: "tenant-a", @@ -175,9 +715,10 @@ func TestEmbeddedCronAdapter_CreateJob(t *testing.T) { } // Default timeout job2, err := adapter.CreateJob(ctx, htools.CronCreateJobRequest{ - Name: "default-timeout", - Schedule: "0 * * * *", - ExecType: "shell", + Name: "default-timeout", + Schedule: "0 * * * *", + ExecType: "shell", + ExecConfig: `{"command":"echo hi"}`, }) if err != nil { t.Fatalf("CreateJob default timeout: %v", err) @@ -225,6 +766,31 @@ func TestEmbeddedCronAdapter_CreateJob_Validation(t *testing.T) { }); err == nil { t.Fatal("expected error for invalid exec_type") } + + // Unsafe execution configurations and non-positive timeouts are rejected + // before persistence, with actionable errors. + for _, tc := range []struct { + name string + req htools.CronCreateJobRequest + want string + }{ + {"empty shell command", htools.CronCreateJobRequest{Name: "x", Schedule: "*/5 * * * *", ExecType: "shell", ExecConfig: `{"command":""}`}, "non-empty command"}, + {"incomplete harness prompt", htools.CronCreateJobRequest{Name: "x", Schedule: "*/5 * * * *", ExecType: "harness", ExecConfig: `{"prompt":""}`}, "non-empty prompt"}, + {"negative timeout", htools.CronCreateJobRequest{Name: "x", Schedule: "*/5 * * * *", ExecType: "shell", ExecConfig: `{"command":"echo hi"}`, TimeoutSec: -1}, "timeout_seconds must be positive"}, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := adapter.CreateJob(ctx, tc.req) + if tc.want != "" { + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("error = %v, want %q", err, tc.want) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } } func TestEmbeddedCronAdapter_GetJob(t *testing.T) { @@ -233,9 +799,10 @@ func TestEmbeddedCronAdapter_GetJob(t *testing.T) { ctx := context.Background() created, err := adapter.CreateJob(ctx, htools.CronCreateJobRequest{ - Name: "get-test", - Schedule: "*/5 * * * *", - ExecType: "shell", + Name: "get-test", + Schedule: "*/5 * * * *", + ExecType: "shell", + ExecConfig: `{"command":"echo hi"}`, }) if err != nil { t.Fatalf("CreateJob: %v", err) @@ -250,8 +817,8 @@ func TestEmbeddedCronAdapter_GetJob(t *testing.T) { t.Fatalf("ID mismatch: got %q, want %q", got.ID, created.ID) } - // Get by name (fallback) - got2, err := adapter.GetJob(ctx, "get-test") + // Explicit operator lookup by name. + got2, err := adapter.GetJobByName(ctx, "get-test") if err != nil { t.Fatalf("GetJob by name: %v", err) } @@ -280,8 +847,8 @@ func TestEmbeddedCronAdapter_ListJobs(t *testing.T) { } // Create two - adapter.CreateJob(ctx, htools.CronCreateJobRequest{Name: "j1", Schedule: "*/5 * * * *", ExecType: "shell"}) - adapter.CreateJob(ctx, htools.CronCreateJobRequest{Name: "j2", Schedule: "0 * * * *", ExecType: "shell"}) + adapter.CreateJob(ctx, htools.CronCreateJobRequest{Name: "j1", Schedule: "*/5 * * * *", ExecType: "shell", ExecConfig: `{"command":"echo hi"}`}) + adapter.CreateJob(ctx, htools.CronCreateJobRequest{Name: "j2", Schedule: "0 * * * *", ExecType: "shell", ExecConfig: `{"command":"echo hi"}`}) jobs, err = adapter.ListJobs(ctx) if err != nil { @@ -298,9 +865,10 @@ func TestEmbeddedCronAdapter_UpdateJob_Schedule(t *testing.T) { ctx := context.Background() created, err := adapter.CreateJob(ctx, htools.CronCreateJobRequest{ - Name: "update-sched", - Schedule: "*/5 * * * *", - ExecType: "shell", + Name: "update-sched", + Schedule: "*/5 * * * *", + ExecType: "shell", + ExecConfig: `{"command":"echo hi"}`, }) if err != nil { t.Fatalf("CreateJob: %v", err) @@ -327,9 +895,10 @@ func TestEmbeddedCronAdapter_UpdateJob_PauseResume(t *testing.T) { ctx := context.Background() created, err := adapter.CreateJob(ctx, htools.CronCreateJobRequest{ - Name: "pause-resume", - Schedule: "*/5 * * * *", - ExecType: "shell", + Name: "pause-resume", + Schedule: "*/5 * * * *", + ExecType: "shell", + ExecConfig: `{"command":"echo hi"}`, }) if err != nil { t.Fatalf("CreateJob: %v", err) @@ -367,9 +936,10 @@ func TestEmbeddedCronAdapter_UpdateJob_Validation(t *testing.T) { } created, _ := adapter.CreateJob(ctx, htools.CronCreateJobRequest{ - Name: "val-test", - Schedule: "*/5 * * * *", - ExecType: "shell", + Name: "val-test", + Schedule: "*/5 * * * *", + ExecType: "shell", + ExecConfig: `{"command":"echo hi"}`, }) // Empty schedule @@ -397,9 +967,10 @@ func TestEmbeddedCronAdapter_DeleteJob(t *testing.T) { ctx := context.Background() created, _ := adapter.CreateJob(ctx, htools.CronCreateJobRequest{ - Name: "delete-me", - Schedule: "*/5 * * * *", - ExecType: "shell", + Name: "delete-me", + Schedule: "*/5 * * * *", + ExecType: "shell", + ExecConfig: `{"command":"echo hi"}`, }) if err := adapter.DeleteJob(ctx, created.ID); err != nil { @@ -421,9 +992,10 @@ func TestEmbeddedCronAdapter_ListExecutions(t *testing.T) { ctx := context.Background() created, _ := adapter.CreateJob(ctx, htools.CronCreateJobRequest{ - Name: "exec-test", - Schedule: "*/5 * * * *", - ExecType: "shell", + Name: "exec-test", + Schedule: "*/5 * * * *", + ExecType: "shell", + ExecConfig: `{"command":"echo hi"}`, }) execs, err := adapter.ListExecutions(ctx, created.ID, 10, 0) @@ -453,7 +1025,7 @@ func TestEmbeddedCronAdapter_Concurrent(t *testing.T) { // Seed a job so concurrent reads/updates have something to hit. seed, _ := adapter.CreateJob(ctx, htools.CronCreateJobRequest{ - Name: "seed-job", Schedule: "*/5 * * * *", ExecType: "shell", + Name: "seed-job", Schedule: "*/5 * * * *", ExecType: "shell", ExecConfig: `{"command":"echo hi"}`, }) for i := 0; i < 10; i++ { @@ -481,9 +1053,10 @@ func TestEmbeddedCronAdapter_Concurrent(t *testing.T) { defer wg.Done() // Writes may hit SQLITE_BUSY under extreme concurrency — acceptable. adapter.CreateJob(ctx, htools.CronCreateJobRequest{ - Name: fmt.Sprintf("concurrent-%d", i), - Schedule: "*/5 * * * *", - ExecType: "shell", + Name: fmt.Sprintf("concurrent-%d", i), + Schedule: "*/5 * * * *", + ExecType: "shell", + ExecConfig: `{"command":"echo hi"}`, }) }() } diff --git a/cmd/harnessd/main.go b/cmd/harnessd/main.go index 617a1624..739ccc80 100644 --- a/cmd/harnessd/main.go +++ b/cmd/harnessd/main.go @@ -1640,16 +1640,20 @@ func (a *cronClientAdapter) GetJob(ctx context.Context, id string) (htools.CronJ func (a *cronClientAdapter) UpdateJob(ctx context.Context, id string, req htools.CronUpdateJobRequest) (htools.CronJob, error) { j, err := a.client.UpdateJob(ctx, id, cron.UpdateJobRequest{ - Schedule: req.Schedule, - ExecConfig: req.ExecConfig, - Status: req.Status, - TimeoutSec: req.TimeoutSec, - Tags: req.Tags, + Schedule: req.Schedule, + ExecConfig: req.ExecConfig, + Status: req.Status, + TimeoutSec: req.TimeoutSec, + Tags: req.Tags, + ExpectedUpdatedAt: req.ExpectedUpdatedAt, }) if err != nil { if cron.IsJobNotFound(err) { return htools.CronJob{}, htools.ErrCronJobNotFound } + if cron.IsJobConflict(err) { + return htools.CronJob{}, htools.ErrCronJobConflict + } return htools.CronJob{}, err } return cronJobFromCron(j), nil @@ -1665,6 +1669,19 @@ func (a *cronClientAdapter) DeleteJob(ctx context.Context, id string) error { return nil } +func (a *cronClientAdapter) DeleteJobCAS(ctx context.Context, id string, expectedUpdatedAt time.Time) error { + if err := a.client.DeleteJobCAS(ctx, id, expectedUpdatedAt); err != nil { + if cron.IsJobNotFound(err) { + return htools.ErrCronJobNotFound + } + if cron.IsJobConflict(err) { + return htools.ErrCronJobConflict + } + return err + } + return nil +} + func (a *cronClientAdapter) ListExecutions(ctx context.Context, jobID string, limit, offset int) ([]htools.CronExecution, error) { execs, err := a.client.ListExecutions(ctx, jobID, limit, offset) if err != nil { @@ -1717,13 +1734,38 @@ func cronExecFromCron(e cron.Execution) htools.CronExecution { // embeddedCronAdapter implements htools.CronClient by calling cron.Store // and cron.Scheduler directly, without HTTP. +type embeddedCronScheduler interface { + cron.JobScheduler + HasEntry(string) bool +} + type embeddedCronAdapter struct { + mu sync.Mutex store cron.Store - scheduler *cron.Scheduler + scheduler embeddedCronScheduler clock cron.Clock } +func (a *embeddedCronAdapter) getJob(ctx context.Context, id string) (cron.Job, error) { + if scope, ok := cron.ScopeFromContext(ctx); ok { + if store, ok := a.store.(cron.ScopedStore); ok { + return store.GetJobInScope(ctx, id, scope) + } + job, err := a.store.GetJob(ctx, id) + if err == nil && !scope.Matches(job) { + return cron.Job{}, cron.ErrJobNotFound + } + return job, err + } + return a.store.GetJob(ctx, id) +} + func (a *embeddedCronAdapter) CreateJob(ctx context.Context, req htools.CronCreateJobRequest) (htools.CronJob, error) { + a.mu.Lock() + defer a.mu.Unlock() + if scope, ok := cron.ScopeFromContext(ctx); ok && (req.TenantID != scope.TenantID || req.ConversationID != scope.ConversationID || req.AgentID != scope.AgentID) { + return htools.CronJob{}, fmt.Errorf("cron create scope does not match request scope") + } if req.Name == "" { return htools.CronJob{}, fmt.Errorf("name is required") } @@ -1737,7 +1779,13 @@ func (a *embeddedCronAdapter) CreateJob(ctx context.Context, req htools.CronCrea if req.ExecType != cron.ExecTypeShell && req.ExecType != cron.ExecTypeHarness { return htools.CronJob{}, fmt.Errorf("execution_type must be \"shell\" or \"harness\"") } - if req.TimeoutSec <= 0 { + if err := cron.ValidateExecutionConfig(req.ExecType, req.ExecConfig); err != nil { + return htools.CronJob{}, err + } + if req.TimeoutSec < 0 { + return htools.CronJob{}, fmt.Errorf("timeout_seconds must be positive") + } + if req.TimeoutSec == 0 { req.TimeoutSec = 30 } now := a.clock.Now() @@ -1750,12 +1798,14 @@ func (a *embeddedCronAdapter) CreateJob(ctx context.Context, req htools.CronCrea Schedule: req.Schedule, ExecType: req.ExecType, ExecConfig: req.ExecConfig, - Status: cron.StatusActive, - TimeoutSec: req.TimeoutSec, - Tags: req.Tags, - NextRunAt: nextRun, - CreatedAt: now, - UpdatedAt: now, + // Persist non-runnable until the live entry exists. A failed registration + // or activation can then never become a restart-rearmable active row. + Status: cron.StatusPaused, + TimeoutSec: req.TimeoutSec, + Tags: req.Tags, + NextRunAt: nextRun, + CreatedAt: now, + UpdatedAt: now, } job, err = a.store.CreateJob(ctx, job) if err != nil { @@ -1764,14 +1814,44 @@ func (a *embeddedCronAdapter) CreateJob(ctx context.Context, req htools.CronCrea } return htools.CronJob{}, fmt.Errorf("store: %w", err) } - if addErr := a.scheduler.AddJob(job); addErr != nil { + activeJob := job + activeJob.Status = cron.StatusActive + if addErr := a.scheduler.AddJob(activeJob); addErr != nil { + a.scheduler.RemoveJob(job.ID) return htools.CronJob{}, fmt.Errorf("scheduler: %w", addErr) } - return cronJobFromCron(job), nil + activeJob.UpdatedAt = a.clock.Now() + if !activeJob.UpdatedAt.After(job.UpdatedAt) { + activeJob.UpdatedAt = job.UpdatedAt.Add(time.Nanosecond) + } + if err := a.store.UpdateJobCAS(ctx, activeJob, job.UpdatedAt); err != nil { + a.scheduler.RemoveJob(job.ID) + return htools.CronJob{}, fmt.Errorf("activate registered job: %w", err) + } + return cronJobFromCron(activeJob), nil } func (a *embeddedCronAdapter) ListJobs(ctx context.Context) ([]htools.CronJob, error) { - jobs, err := a.store.ListJobs(ctx) + var jobs []cron.Job + var err error + if scope, ok := cron.ScopeFromContext(ctx); ok { + if store, ok := a.store.(cron.ScopedStore); ok { + jobs, err = store.ListJobsInScope(ctx, scope) + } else { + jobs, err = a.store.ListJobs(ctx) + if err == nil { + filtered := jobs[:0] + for _, job := range jobs { + if scope.Matches(job) { + filtered = append(filtered, job) + } + } + jobs = filtered + } + } + } else { + jobs, err = a.store.ListJobs(ctx) + } if err != nil { return nil, err } @@ -1783,30 +1863,40 @@ func (a *embeddedCronAdapter) ListJobs(ctx context.Context) ([]htools.CronJob, e } func (a *embeddedCronAdapter) GetJob(ctx context.Context, id string) (htools.CronJob, error) { - job, err := a.store.GetJob(ctx, id) + job, err := a.getJob(ctx, id) if err != nil { - if !cron.IsJobNotFound(err) { - return htools.CronJob{}, err + if cron.IsJobNotFound(err) { + return htools.CronJob{}, htools.ErrCronJobNotFound } - job, err = a.store.GetJobByName(ctx, id) - if err != nil { - if cron.IsJobNotFound(err) { - return htools.CronJob{}, htools.ErrCronJobNotFound - } - return htools.CronJob{}, err + return htools.CronJob{}, err + } + return cronJobFromCron(job), nil +} + +// GetJobByName is reserved for explicit operator lookup; model-facing CRUD +// reaches GetJob and remains ID-only. +func (a *embeddedCronAdapter) GetJobByName(ctx context.Context, name string) (htools.CronJob, error) { + job, err := a.store.GetJobByName(ctx, name) + if err != nil { + if cron.IsJobNotFound(err) { + return htools.CronJob{}, htools.ErrCronJobNotFound } + return htools.CronJob{}, err } return cronJobFromCron(job), nil } func (a *embeddedCronAdapter) UpdateJob(ctx context.Context, id string, req htools.CronUpdateJobRequest) (htools.CronJob, error) { - job, err := a.store.GetJob(ctx, id) + a.mu.Lock() + defer a.mu.Unlock() + job, err := a.getJob(ctx, id) if err != nil { if cron.IsJobNotFound(err) { return htools.CronJob{}, htools.ErrCronJobNotFound } return htools.CronJob{}, err } + originalJob := job if req.Schedule != nil { trimmed := strings.TrimSpace(*req.Schedule) @@ -1824,27 +1914,23 @@ func (a *embeddedCronAdapter) UpdateJob(ctx context.Context, id string, req htoo job.ExecConfig = *req.ExecConfig } if req.TimeoutSec != nil { + if *req.TimeoutSec <= 0 { + return htools.CronJob{}, fmt.Errorf("timeout_seconds must be positive") + } job.TimeoutSec = *req.TimeoutSec } if req.Tags != nil { job.Tags = *req.Tags } + if err := cron.ValidateExecutionConfig(job.ExecType, job.ExecConfig); err != nil { + return htools.CronJob{}, err + } if req.Status != nil { if *req.Status != cron.StatusActive && *req.Status != cron.StatusPaused { return htools.CronJob{}, fmt.Errorf("status must be \"active\" or \"paused\"") } - oldStatus := job.Status job.Status = *req.Status - - if *req.Status == cron.StatusPaused && oldStatus != cron.StatusPaused { - a.scheduler.RemoveJob(job.ID) - } - if *req.Status == cron.StatusActive && oldStatus != cron.StatusActive { - if addErr := a.scheduler.AddJob(job); addErr != nil { - return htools.CronJob{}, fmt.Errorf("scheduler: %w", addErr) - } - } } // Gate on job.Status (the EFFECTIVE post-update status), not on @@ -1852,23 +1938,56 @@ func (a *embeddedCronAdapter) UpdateJob(ctx context.Context, id string, req htoo // internal/cron/server.go's handleUpdateJob. A schedule-only update // (req.Status == nil) must not re-arm a job whose stored status is // paused: job.Status already reflects that live status in that case. - if req.Schedule != nil && job.Status == cron.StatusActive { - if err := a.scheduler.UpdateJobSchedule(job); err != nil { + expectedUpdatedAt := job.UpdatedAt + if req.ExpectedUpdatedAt != nil { + expectedUpdatedAt = req.ExpectedUpdatedAt.UTC() + } + job.UpdatedAt = a.clock.Now() + if !job.UpdatedAt.After(expectedUpdatedAt) { + job.UpdatedAt = expectedUpdatedAt.Add(time.Nanosecond) + } + scheduleChanged := req.Schedule != nil + twoPhaseActivate := job.Status == cron.StatusActive && (scheduleChanged || originalJob.Status != cron.StatusActive) + if twoPhaseActivate { + prepared, err := a.scheduler.PrepareJob(job) + if err != nil { return htools.CronJob{}, fmt.Errorf("scheduler: %w", err) } + if err := a.store.UpdateJobCAS(ctx, job, expectedUpdatedAt); err != nil { + a.scheduler.AbortJob(prepared) + if cron.IsJobConflict(err) { + return htools.CronJob{}, htools.ErrCronJobConflict + } + return htools.CronJob{}, fmt.Errorf("store: %w", err) + } + a.scheduler.CommitJob(prepared) + return cronJobFromCron(job), nil } - job.UpdatedAt = a.clock.Now() - if err := a.store.UpdateJob(ctx, job); err != nil { + if err := a.store.UpdateJobCAS(ctx, job, expectedUpdatedAt); err != nil { if cron.IsJobNotFound(err) { return htools.CronJob{}, htools.ErrCronJobNotFound } + if cron.IsJobConflict(err) { + return htools.CronJob{}, htools.ErrCronJobConflict + } return htools.CronJob{}, fmt.Errorf("store: %w", err) } + if job.Status == cron.StatusPaused { + a.scheduler.RemoveJob(job.ID) + } return cronJobFromCron(job), nil } func (a *embeddedCronAdapter) DeleteJob(ctx context.Context, id string) error { + a.mu.Lock() + defer a.mu.Unlock() + if _, err := a.getJob(ctx, id); err != nil { + if cron.IsJobNotFound(err) { + return htools.ErrCronJobNotFound + } + return err + } if err := a.store.DeleteJob(ctx, id); err != nil { if cron.IsJobNotFound(err) { return htools.ErrCronJobNotFound @@ -1879,7 +1998,35 @@ func (a *embeddedCronAdapter) DeleteJob(ctx context.Context, id string) error { return nil } +func (a *embeddedCronAdapter) DeleteJobCAS(ctx context.Context, id string, expectedUpdatedAt time.Time) error { + a.mu.Lock() + defer a.mu.Unlock() + if _, err := a.getJob(ctx, id); err != nil { + if cron.IsJobNotFound(err) { + return htools.ErrCronJobNotFound + } + return err + } + if err := a.store.DeleteJobCAS(ctx, id, expectedUpdatedAt.UTC()); err != nil { + if cron.IsJobNotFound(err) { + return htools.ErrCronJobNotFound + } + if cron.IsJobConflict(err) { + return htools.ErrCronJobConflict + } + return err + } + a.scheduler.RemoveJob(id) + return nil +} + func (a *embeddedCronAdapter) ListExecutions(ctx context.Context, jobID string, limit, offset int) ([]htools.CronExecution, error) { + if _, err := a.getJob(ctx, jobID); err != nil { + if cron.IsJobNotFound(err) { + return nil, htools.ErrCronJobNotFound + } + return nil, err + } execs, err := a.store.ListExecutions(ctx, jobID, limit, offset) if err != nil { return nil, err diff --git a/cmd/harnessd/main_test.go b/cmd/harnessd/main_test.go index 3d2e6180..d5f6953b 100644 --- a/cmd/harnessd/main_test.go +++ b/cmd/harnessd/main_test.go @@ -1409,7 +1409,7 @@ func sampleJob() cron.Job { Name: "test-job", Schedule: "*/5 * * * *", ExecType: "shell", - ExecConfig: `{"cmd":"echo hi"}`, + ExecConfig: `{"command":"echo hi"}`, Status: "active", TimeoutSec: 60, Tags: "test", @@ -1586,6 +1586,9 @@ func TestCronClientAdapterUpdateJob(t *testing.T) { if reqBody["schedule"] != "0 * * * *" { t.Errorf("request schedule: got %v, want %q", reqBody["schedule"], "0 * * * *") } + if reqBody["expected_updated_at"] != "2026-07-31T00:00:00Z" { + t.Errorf("request expected_updated_at: got %v, want %q", reqBody["expected_updated_at"], "2026-07-31T00:00:00Z") + } _ = json.NewEncoder(w).Encode(updatedJob) })) defer ts.Close() @@ -1593,9 +1596,11 @@ func TestCronClientAdapterUpdateJob(t *testing.T) { adapter := newTestAdapter(ts) newSched := "0 * * * *" newStatus := "paused" + expectedUpdatedAt := time.Date(2026, 7, 31, 0, 0, 0, 0, time.UTC) got, err := adapter.UpdateJob(context.Background(), "job-abc", htools.CronUpdateJobRequest{ - Schedule: &newSched, - Status: &newStatus, + Schedule: &newSched, + Status: &newStatus, + ExpectedUpdatedAt: &expectedUpdatedAt, }) if err != nil { t.Fatalf("UpdateJob: %v", err) @@ -2394,7 +2399,7 @@ func TestEmbeddedCronAdapterListJobs(t *testing.T) { // Create a job first. _, err := adapter.CreateJob(context.Background(), htools.CronCreateJobRequest{ - Name: "list-test", Schedule: "*/5 * * * *", ExecType: "shell", + Name: "list-test", Schedule: "*/5 * * * *", ExecType: "shell", ExecConfig: `{"command":"echo hi"}`, }) if err != nil { t.Fatalf("CreateJob: %v", err) @@ -2420,7 +2425,7 @@ func TestEmbeddedCronAdapterGetJob(t *testing.T) { adapter := &embeddedCronAdapter{store: store, scheduler: scheduler, clock: clock} created, err := adapter.CreateJob(context.Background(), htools.CronCreateJobRequest{ - Name: "get-test", Schedule: "*/5 * * * *", ExecType: "shell", + Name: "get-test", Schedule: "*/5 * * * *", ExecType: "shell", ExecConfig: `{"command":"echo hi"}`, }) if err != nil { t.Fatalf("CreateJob: %v", err) @@ -2435,8 +2440,8 @@ func TestEmbeddedCronAdapterGetJob(t *testing.T) { t.Fatalf("Name: got %q", got.Name) } - // Get by name. - got, err = adapter.GetJob(context.Background(), "get-test") + // Explicit operator lookup by name. + got, err = adapter.GetJobByName(context.Background(), "get-test") if err != nil { t.Fatalf("GetJob by name: %v", err) } @@ -2465,7 +2470,7 @@ func TestEmbeddedCronAdapterUpdateJob(t *testing.T) { adapter := &embeddedCronAdapter{store: store, scheduler: scheduler, clock: clock} created, err := adapter.CreateJob(context.Background(), htools.CronCreateJobRequest{ - Name: "update-test", Schedule: "*/5 * * * *", ExecType: "shell", + Name: "update-test", Schedule: "*/5 * * * *", ExecType: "shell", ExecConfig: `{"command":"echo hi"}`, }) if err != nil { t.Fatalf("CreateJob: %v", err) @@ -2556,7 +2561,7 @@ func TestEmbeddedCronAdapterUpdateJob_PausedJobScheduleOnlyPatch_NotReArmed(t *t adapter := &embeddedCronAdapter{store: store, scheduler: scheduler, clock: clock} created, err := adapter.CreateJob(context.Background(), htools.CronCreateJobRequest{ - Name: "pause-then-schedule-only", Schedule: "*/5 * * * *", ExecType: "shell", + Name: "pause-then-schedule-only", Schedule: "*/5 * * * *", ExecType: "shell", ExecConfig: `{"command":"echo hi"}`, }) if err != nil { t.Fatalf("CreateJob: %v", err) @@ -2607,7 +2612,7 @@ func TestEmbeddedCronAdapterUpdateJob_ResumeAndScheduleReArms(t *testing.T) { adapter := &embeddedCronAdapter{store: store, scheduler: scheduler, clock: clock} created, err := adapter.CreateJob(context.Background(), htools.CronCreateJobRequest{ - Name: "resume-and-schedule", Schedule: "*/5 * * * *", ExecType: "shell", + Name: "resume-and-schedule", Schedule: "*/5 * * * *", ExecType: "shell", ExecConfig: `{"command":"echo hi"}`, }) if err != nil { t.Fatalf("CreateJob: %v", err) @@ -2651,7 +2656,7 @@ func TestEmbeddedCronAdapterDeleteJob(t *testing.T) { adapter := &embeddedCronAdapter{store: store, scheduler: scheduler, clock: clock} created, err := adapter.CreateJob(context.Background(), htools.CronCreateJobRequest{ - Name: "delete-test", Schedule: "*/5 * * * *", ExecType: "shell", + Name: "delete-test", Schedule: "*/5 * * * *", ExecType: "shell", ExecConfig: `{"command":"echo hi"}`, }) if err != nil { t.Fatalf("CreateJob: %v", err) @@ -2679,7 +2684,7 @@ func TestEmbeddedCronAdapterListExecutions(t *testing.T) { adapter := &embeddedCronAdapter{store: store, scheduler: scheduler, clock: clock} created, err := adapter.CreateJob(context.Background(), htools.CronCreateJobRequest{ - Name: "exec-test", Schedule: "*/5 * * * *", ExecType: "shell", + Name: "exec-test", Schedule: "*/5 * * * *", ExecType: "shell", ExecConfig: `{"command":"echo hi"}`, }) if err != nil { t.Fatalf("CreateJob: %v", err) diff --git a/docs/logs/engineering-log.md b/docs/logs/engineering-log.md index 22a1b114..11c3cea5 100644 --- a/docs/logs/engineering-log.md +++ b/docs/logs/engineering-log.md @@ -80,6 +80,99 @@ that baseline test passed 20 normal and 10 race repetitions locally and is being rechecked independently rather than waived. +## 2026-08-01 (Conversational Cron CRUD Acceptance Repair — Issue #1002) + +- Symptoms: model GET could fall through to global name lookup; same-name global lookup selected an arbitrary scope; active resume leaked a second robfig entry; scheduler failure followed persistence without rollback; migration recognized only a narrow `UNIQUE` spelling. +- Final lifecycle repair: create and paused→active resume use paused-first registration so failure remains restart-safe. Active schedule replacement uses inert `Prepare` → durable CAS → infallible `Commit`, preserving the prior active row/entry on prepare or CAS failure. Registration identities are globally monotonic; after jitter and durable reload, identity validation and `CreateExecution` form one scheduler-locked admission point shared with prepare/commit/remove. +- Deterministic reds: embedded model get accepted `shared-name`; the ID route invoked name lookup; global same-name lookup lacked `IsJobAmbiguous`; duplicate add left two live entries; quoted/bracketed/backtick migrations retained global uniqueness. +- Fix: split `/v1/jobs/{id}` from explicit `/v1/jobs/by-name?name=...`, add typed ambiguity, put remote/embedded ownership in SQLite predicates, replace old live entries through prepared scheduler transactions, and broaden transactional migration recognition. Query encoding preserves arbitrary non-empty names, including slash, spaces, percent, and Unicode. +- Durable proof: a four-variant legacy matrix preserves two jobs, two execution rows, run metadata, exact timestamps, and scoped uniqueness across an idempotent second migrate; `integrity_check` and `foreign_key_check` pass. +- Earlier candidate evidence (superseded by the lifecycle follow-ups below): remote CRUD/history and concurrent update/delete tests passed the then-current focused packages. It is not latest-head broad/full regression evidence. +- Review follow-up: `url.PathEscape` could not preserve a slash once Go exposed + decoded `URL.Path`. The exact operator route now reads `name` from the query, + rejects empty input at client/server boundaries, and advertises GET only. + Slash/space/percent/Unicode regressions passed normal/race; complete + `internal/cron` passed normal in 8.783s and race in 10.807s. +- Blocking review follow-up: production registries captured the raw cron + adapter, pause/resume omitted the model's read version, and DDL regex parsing + missed legal `UNIQUE(name COLLATE NOCASE)`. The mutation audit found the same + stale-write gap on model delete. +- Deterministic reds: assembled embedded and remote default registries both let + scope B read scope A; pause/resume/delete schemas required only `id`; stale + model delete succeeded; and the semantic index inspector was absent while + the collated migration variant retained global uniqueness. +- Fix: `NewDefaultRegistryWithOptions` now applies one idempotent scoped client, + which covers top-level, worktree per-run, and subagent registry construction + while operator adapters remain raw. Pause/resume/delete require + `expected_updated_at`; remote and embedded delete use persistence CAS and + return typed conflict. Migration now uses SQLite `index_list`/`index_xinfo` + key metadata and ignores composite or partial uniqueness. +- Superseded lifecycle-convergence chronology: deterministic remote and embedded reds used + scheduler add/replacement failures plus an injected `TouchJobRun` between the + successful write and rollback CAS; both left an active durable row divergent + from live dispatch. Separate create reds made scheduler registration and + compensating delete fail, leaving an active stored orphan. +- Superseded fix chronology: both adapters called a rollback recovery policy. Rollback conflict + reloads durable authority and re-registers that exact active row; persistent + registration failure calls atomic `DeactivateJob`, which changes only status + and version, then removes live dispatch. Failed create deletion uses the same + durable deactivation. This design and the dead `DeactivateJob` API were later + replaced by prepared scheduler transactions. +- Lifecycle-convergence verification: the bounded nine-package normal command + passed in 8.993s/11.698s/1.642s/9.359s/1.307s/1.547s/4.757s/5.893s/3.169s; + the same package set with `-race` passed in + 11.223s/12.901s/2.103s/10.298s/1.416s/2.454s/4.639s/7.720s/4.077s. No full + regression, staging, commit, rebase, push, server launch, or GitHub mutation + was authorized. +- Durable proof: the five-variant migration matrix preserves jobs, executions, + timestamps, foreign keys, and integrity across an idempotent second migrate; + assembled embedded/remote registries reject cross-scope and stale mutations + without a manual wrapper. +- Focused verification: `go test ./internal/cron ./internal/harness/tools/... ./internal/harness ./cmd/harnessd -count=1` passed all nine emitted packages in 9.284s/11.486s/1.553s/9.323s/1.295s/1.573s/4.473s/6.469s/3.333s; the same bounded package set with `-race` passed in 10.958s/12.295s/2.499s/10.693s/1.971s/2.268s/4.424s/6.985s/3.122s. No full regression, live server, commit, push, or GitHub mutation was authorized. +- Final read-only review found that `cron_get` converted every history retrieval + error into `recent_executions: []` without an availability signal, letting a + model mistake database failure for proof that a job never ran. The new red + required explicit unavailable state and warning while keeping the job and + backward-compatible empty array. The tool now emits + `recent_executions_available` on every result and + `recent_executions_warning` on failure; its description documents the + interpretation rule. +- Latest admission-lock and history-availability verification: + `go test ./internal/cron ./internal/harness ./internal/harness/tools ./internal/harness/tools/deferred ./cmd/harnessd -count=1` + passed in 9.522s/5.596s/11.403s/9.466s/2.402s; the same five packages with + `-race` passed in 11.202s/8.741s/12.588s/10.954s/4.512s. The focused history + red failed for missing availability/warning fields before production code; + its normal/race green passed in 0.330s/1.349s. The candidate was then rebased + onto `origin/main` `3506e01c`. +- Live provider compatibility follow-up: an OpenAI-compatible harness canary + rejected `cron_create` before model execution because the function schema + carried top-level `oneOf`. Those providers require the top-level schema to + be a plain object and reject composition keywords there. The schema now + advertises the optional shell `command` and harness `prompt` fields without + top-level composition; the existing handler remains the fail-closed authority + for execution-type pairing and required non-empty payloads. The focused + regression asserts every provider-forbidden top-level composition keyword is + absent while retaining the required object root. +- Final exact-tree verification: the foreground `./scripts/test-regression.sh` + passed normal, complete race, and coverage at 85.7% total with zero uncovered + production functions. The assembled core-registry regression also checks + every visible root schema and explicitly covers all eight cron tools. +- Real-provider proof: OpenAI `gpt-4.1-mini` invoked all eight model-facing cron + tools in one persisted conversation. The first job fired twice into distinct + scheduler-started runs whose assistant output appeared in conversation SSE + and transcript. A stale update version was rejected; a fresh update moved the + schedule to 2027 and changed the harness prompt; get/history returned both + execution records with linked run IDs; pause/resume changed durable status; + and versioned delete ended with `jobs: []` plus HTTP 404 for the former ID. + All eleven runs completed with the exact tenant/conversation/agent tuple. +- Final promotion-review repair: `cron_create` still told the model to use + `bash` plus `sleep` for one-shot delayed work, contradicting the core-visible + `set_delayed_callback` path and its same-conversation continuation intent. + The description regression failed on the old guidance, then passed normal + and race after routing one-shot conversational work to + `set_delayed_callback`. Frontier review identified the defect and an + independent cheap-agent exact-diff re-review returned CLEAR. + ## 2026-07-31 (Workflow Initial Write Exit Arbitration — Issue #1076) - Symptom: hosted `test-race` run `30660042116` reported only diff --git a/docs/logs/long-term-thinking-log.md b/docs/logs/long-term-thinking-log.md index 776c7081..125e0b9e 100644 --- a/docs/logs/long-term-thinking-log.md +++ b/docs/logs/long-term-thinking-log.md @@ -38,6 +38,18 @@ reducer change, no server/provider/schema expansion, and no failing-baseline waiver. +## 2026-08-01 (Conversational Cron CRUD Acceptance Repair — Issue #1002) + +- Command intent: close the exact-head audit blockers in conversational cron + CRUD/history, prove the real same-conversation lifecycle, and promote the + issue through its guarded PR and merge gates. +- User intent: names may repeat across independent tenant/conversation/agent scopes, while every model mutation/read remains ID-only and ownership-safe; durable and live scheduler state must never silently diverge. +- Success definition: deterministic reds cover name fallback, ambiguous operator lookup, assembled automatic embedded/remote model scope, stale edit/pause/resume/delete, semantic collated migration detection, prepared active replacement, non-reusable registration identity, and linearizable execution admission; migration preserves jobs/history/timestamps and integrity; full regression and a real-provider all-eight-tool same-conversation fire/CRUD lifecycle pass before promotion. +- Non-goals: #1003 authentication/readiness, #1004 overlap/terminal linkage, + callbacks, and native UI. No regression or hosted failure is waived. +- Guardrails: preserve the existing candidate, use strict focused TDD, scope at the common model-registry boundary exactly once, retain raw operator compatibility, keep operator name lookup distinct from model CRUD, require a read version for every existing-row model mutation, and return not-found across scope boundaries. +- Final guardrail: create and resume are paused-first. Active replacement retains the old live/durable job through inert `Prepare`, then CASes the new active row and performs an infallible commit. The final registration check and execution-row creation are one scheduler-locked admission point shared with prepare/commit/remove. Overlapping run-tracking monotonicity belongs to #1004 and is not folded into #1002. + ## 2026-07-31 (Workflow Initial Write Exit Arbitration — Issue #1076) - Command intent: repair the separate hosted race failure where the initial diff --git a/docs/logs/observational-log.md b/docs/logs/observational-log.md index 79fdd4d1..508c7335 100644 --- a/docs/logs/observational-log.md +++ b/docs/logs/observational-log.md @@ -34,6 +34,35 @@ Use this file for observations about system behavior without immediately prescri without clearing the accumulator made either terminal path re-export the previous run's reply; run start must reset both pieces of per-run ownership. +## 2026-08-01 (Conversational Cron Identity and Lifecycle) + +- Identity observation: a human-readable cron name is scoped display identity, not a globally stable mutation key; model CRUD/history needs the generated job ID. +- Lookup observation: global name lookup becomes intrinsically ambiguous once independent scopes may share conventional names, so arbitrary first-row selection is data leakage rather than convenience. +- Scheduler observation: the entries map is not the live robfig registry; overwriting its value without removing the former entry hides duplicate future fires. +- Atomicity observation: an inert prepared replacement retains the old entry; + durable CAS then an infallible in-memory commit makes failure preserve the + original active job without rollback writes. +- Lifecycle ordering observation: create and resume are paused-first, but + active replacement must not stage paused. Monotonic registration identities + plus a final post-jitter/reload guard suppress queued stale callbacks after + pause/resume or replacement. +- Registry observation: putting scope only in a helper does not protect live model tools; the shared registry constructor is the boundary common to top-level, worktree per-run, and subagent catalogs. Idempotent wrapping prevents callers from stacking redundant scope clients. +- Concurrency observation: pause, resume, and delete are mutations of the same versioned row as edit; ID-only lookup without the `updated_at` read token still permits stale intent. +- Migration observation: SQLite DDL text is not a semantic API. `index_list` plus `index_xinfo` identifies inline, named, quoted, and collated one-column uniqueness while distinguishing composite and partial indexes. +- Provider observation: a direct handler test can pass while the model path is + unusable. OpenAI rejected `cron_create` before inference when its function + schema used root `oneOf`; a plain object schema plus fail-closed handler + validation works across the actual provider boundary. +- Conversation observation: the scheduled executor started two new run IDs + with the exact original tenant/conversation/agent tuple. Conversation SSE + remained open across terminal runs and emitted each `run.started` and final + `assistant.message`; durable messages appended both continuations to the same + transcript. +- CAS observation: `last_run_at` updates the job version. An update using the + prior fire's version conflicted after the next minute fired, while a retry + using the fresh version succeeded. This is visible concurrency protection, + not a test-only branch. + ## 2026-07-31 (Source-Workflow Initial Write Lifecycle) - Lifecycle observation: a successful `cmd.Start` transfers child ownership to diff --git a/docs/logs/system-log.md b/docs/logs/system-log.md index f8dcad1a..d600cff9 100644 --- a/docs/logs/system-log.md +++ b/docs/logs/system-log.md @@ -31,6 +31,19 @@ - Server emission, persistence, authentication, provider behavior, and other clients remain unchanged. +## 2026-08-01 (Conversational Cron CRUD Ownership — Issue #1002) + +- Components: every model registry constructor -> one idempotent scoped cron client -> deferred `cron_*` tools -> embedded adapter or HTTP client/server -> `SQLiteStore` -> `Scheduler`. Operator/server endpoints retain the raw adapter outside this boundary. +- Identity contract: model get/update/history/pause/resume/delete accept job IDs only. Explicit operator name lookup uses `/v1/jobs/by-name?name=...`; query encoding round-trips every non-empty allowed name. Unscoped collisions return typed `ErrJobAmbiguous`, while scoped lookup selects by the complete ownership tuple. +- Persistence contract: non-deleted names are unique within `(tenant_id, conversation_id, agent_id)`. SQLite index metadata identifies a non-partial, one-key-column global name constraint regardless of DDL spelling/collation; legacy global uniqueness is transactionally rebuilt with jobs and executions copied before old tables are dropped. +- Lifecycle contract: create and paused→active resume are paused-first, so registration or activation failure retains a paused restart-safe row. Active schedule replacement is inert `Prepare` → durable CAS → infallible in-memory `Commit`; failed prepare/CAS leaves the old durable row and live entry untouched. Registration identities are monotonic and are checked again after jitter/reload before execution allocation, suppressing queued stale callbacks. Pause/delete remove live dispatch under the same mutation lock. +- Concurrency/security boundary: remote and embedded model paths apply tenant/conversation/agent predicates at store lookup before reading history or mutating. Update/pause/resume/delete require the version returned by `cron_get`; stale model calls return typed conflict/HTTP 409. Concurrent update/delete serializes to either update-then-delete or delete-then-not-found, never a post-delete re-arm. Authentication of raw cronsd requests remains #1003. +- Provider boundary: every initially visible tool schema has a top-level object + shape without provider-forbidden root composition. Shell-versus-harness + pairing remains enforced by the cron handler, so provider compatibility does + not weaken execution validation. +- Compatibility/rollback: existing jobs/history and shell/harness payloads remain readable. Operator callers must use the distinct name route; reverting this slice requires restoring the global identity policy only if duplicate scoped names have not been created. + ## 2026-07-31 (Source-Workflow Initial Write Lifecycle) - System/component: `internal/workflow.SourceManager.runSourceWorkflow`, the diff --git a/docs/plans/2026-07-31-issue-1002-conversational-cron-crud-impact-map.md b/docs/plans/2026-07-31-issue-1002-conversational-cron-crud-impact-map.md new file mode 100644 index 00000000..f4f3fd92 --- /dev/null +++ b/docs/plans/2026-07-31-issue-1002-conversational-cron-crud-impact-map.md @@ -0,0 +1,81 @@ +# Issue #1002 — Conversational Cron CRUD Impact Map + +## Task + +- Task / issue: [#1002](https://github.com/dennisonbertram/go-code/issues/1002) +- Plan link: `2026-07-31-issue-1002-conversational-cron-crud-plan.md` +- Owner: Codex implementation worktree +- Status: acceptance-audit repair implemented locally on + `codex/issue-1002-repair-v2`, review-clear and rebased onto `origin/main` + `3506e01c`; exact rebased full regression and a real-provider + same-conversation CRUD/fire canary are green. Guarded push and merge remain. + +## Current Ownership, Callers, and Data Flow + +- Entry points: `internal/harness/tools/deferred/cron.go` model-facing CRUD tools, `NewDefaultRegistryWithOptions`, `internal/cron.Server` create/PATCH/DELETE routes, `internal/cron.Client`, and the embedded `harnessd` adapter. +- Source of truth: `tools.CronClient`, typed create/update request payloads, and the cron store; the model-facing scoped client owns RunMetadata authorization, while the service owns validation, persistence CAS, and scheduler reconciliation. +- Callers/consumers: model tool registry, embedded `harnessd` adapter, remote cron client, cron HTTP server, SQLite store, and transcript-readable tool result serialization. +- Similar abstractions searched: all `cron_*` constructors, registry wiring, `CronClient` implementations, `RunStartRequest`, `HarnessExecutor`, `DispatchExecutor`, create/update request call sites, permission/catalog tests, and embedded descriptions. The historical `fix/cron-tools-core` branch contains an unmerged `cron_update` implementation; this slice owns the model CRUD/history catalog, while #1003 remote authentication and dispatch remain explicitly excluded. +- Search evidence: `rg -n "cron_(create|list|status|history|update|pause|resume|delete)|CronClient|CronUpdateJobRequest|UpdateJobRequest" internal cmd .github docs`. +- Conclusion: extend the existing update service and tool seam; do not add a dispatcher or parallel cron abstraction. + +## Config, API, CLI, and Tools + +- User-facing config: model schema now distinguishes legacy `shell` command creation from explicit `harness` prompt creation. +- Defaults: omitted create `execution_type` remains legacy shell; omitted create timeout remains 30; explicitly supplied create and all update timeouts must be positive. Shell configs require a non-empty `command`; harness configs require a non-empty `prompt`. +- Environment/config: none. +- API: additive `expected_updated_at` on cron update and versioned delete requests; persistence CAS returns typed conflict and HTTP 409 on zero matching rows. `/v1/jobs/{id}` is now ID-only; explicit operator lookup is `/v1/jobs/by-name?name=...`, whose query encoding preserves slash, spaces, percent, and Unicode. Ownership scope is propagated across the internal client/server request without adding #1003 authentication. Empty raw operator DELETE remains compatible. +- Tool: `cron_create` supports legacy shell or explicit harness prompt config; get/update/history/pause/resume/delete schemas explicitly require job IDs and reject name semantics. Every model mutation of an existing row requires `updated_at` from `cron_get`. `cron_get` always reports whether its recent-history query was available and includes a warning on failure, so an unavailable query cannot masquerade as a successful empty history. +- Errors: missing ID/no-op/invalid timestamp are actionable model-facing errors; global operator name collisions return typed `ErrJobAmbiguous`/HTTP 409. + +## Persistence and Compatibility + +- Schema/migrations: replace legacy global `UNIQUE(name)` with a partial unique index over `(tenant_id, conversation_id, agent_id, name)` for non-deleted rows. Transactional rebuild uses SQLite `index_list`/`index_xinfo` metadata rather than DDL text, recognizing inline/named/quoted/collated single-column constraints while excluding composite and partial indexes; jobs/executions are preserved. +- Compatibility: old stored rows and histories remain readable with exact timestamps; a second migration is a no-op. The former implicit GET-by-name route is deliberately replaced by a distinct operator route so model calls cannot become name lookups. +- Mixed-version behavior: an older remote service may accept name fallback or unversioned PATCH; the current model client requires IDs/version tokens. Raw remote authentication remains #1003. + +## Lifecycle, Security, and Reliability + +- Concurrency: service writes through `UpdateJobCAS` and model deletion through `DeleteJobCAS`; mutation paths serialize store and scheduler transitions. Active replacement is `Prepare` (inert candidate, old entry retained) → durable CAS → infallible in-memory `Commit`; prepare/CAS failure aborts only the candidate. Registration identities are monotonic and checked again after jitter and durable reload, so queued pre-pause/pre-replacement callbacks cannot execute after a completed transition. +- Validation: `ValidateExecutionConfig` is shared by HTTP and embedded service boundaries, so shell-only rows cannot be created with empty/unknown command config and harness rows cannot be created or updated with an incomplete prompt. Model-tool timeout presence is pointer-valued so explicit zero is not confused with the omitted default. +- Security/privacy: no tenant, agent, conversation, or ownership mutation fields are exposed. Remote server and embedded adapter select by exact tenant + conversation + agent predicates before CRUD/history; the wrapper remains a fail-closed outer boundary. Raw cronsd authentication is not added here. +- Failure/recovery: active replacement prepare or CAS failure leaves the prior durable and live state unchanged. Create and paused→active resume persist/register through paused-first handling, so registration/activation failure leaves a durable paused row that restart will not arm. The remote server and embedded adapter share this policy. + +## Product and Integration Surfaces + +- Server/runtime: `NewDefaultRegistryWithOptions` installs one idempotent scoped cron wrapper for every top-level, worktree per-run, and subagent model registry; operator/server wiring retains raw adapters. Cron HTTP server and embedded/remote `harnessd` adapters gain atomic update/delete handling; embedded bootstrap already routes `ExecTypeHarness` through `HarnessExecutor` and the model tool now produces its typed prompt config. +- TUI/web/macOS/other clients: `None — this slice adds a model-facing tool and additive server request field; downstream UI lifecycle work belongs to #1009.` +- Provider/model/tool catalog: registry, permissions metadata, schema, and embedded descriptions cover both `cron_create` execution modes and `cron_update`; no provider routing changes. +- External systems: `None — no new external integration.` +- UX/accessibility: transcript result is the existing structured `CronJob`; no UI surface changes. + +## Deployment and Operations + +- Order/flags: deploy additively; the scoped-name SQLite migration runs at startup. Back up the stopped process's SQLite database before first rollout; no feature flag is required. +- Observability: existing cron service errors and job timestamps remain the diagnostics; no prompt/config logging added. +- Rollback: remove the registry entry/tool implementation while retaining the additive request field; existing CRUD endpoints and stored jobs continue working. +- Runbooks: `None — no operator procedure changes.` + +## Regression Tests + +- Characterization/red: deferred create harness test initially showed shell-only creation; CAS concurrency test initially allowed serial read/check/write behavior; timeout/version/lifecycle tests were added before their fixes. +- Acceptance: typed harness create, immutable scope, same-conversation starter, partial update, atomic stale conflict, no-op/timeout rejection, stable ID/result, registry presence, and full lifecycle. +- Edge/negative: missing ID/version, malformed JSON/timestamp/timeout, client errors, omitted fields, mixed shell/harness inputs, and existing invalid schedule path. +- Integration: real assembled default registries over embedded and remote client/server/SQLite/scheduler adapters prove automatic RunMetadata scope, raw operator compatibility, stale pause/resume/delete conflicts, owned CRUD/history, two-scope same-name isolation, explicit operator ambiguity, paused-first create/resume, prepare/CAS/commit replacement, stale-callback suppression, and concurrent update/delete no-rearm. +- Latest focused commands for this repair: + `go test [ -race ] ./internal/cron ./internal/harness ./internal/harness/tools ./internal/harness/tools/deferred ./cmd/harnessd -count=1`. + Both pass after the history-availability repair. The exact rebased candidate + also passes `./scripts/test-regression.sh` at 85.7% total coverage and zero + uncovered functions. A real OpenAI-backed conversation exercised all eight + model tools and two scheduled same-chat continuations before deletion. Push + and merge remain promotion gates. + +## Documentation and Handoff + +- Specs/public docs: this plan and the embedded `cron_update.md` description. +- After code: plan/index plus long-term-thinking, engineering, observational, and system logs record the audit repair and exact focused evidence. +- Training/release notes: `None — no separate release note system is used for this internal tool catalog change.` + +## Warning Check + +All surfaces are explicitly mapped; unaffected UI/external/runbook surfaces include search-based rationale. diff --git a/docs/plans/2026-07-31-issue-1002-conversational-cron-crud-plan.md b/docs/plans/2026-07-31-issue-1002-conversational-cron-crud-plan.md new file mode 100644 index 00000000..f556a47a --- /dev/null +++ b/docs/plans/2026-07-31-issue-1002-conversational-cron-crud-plan.md @@ -0,0 +1,148 @@ +# Issue #1002 — Conversational Cron CRUD + +## Context + +- Governing GitHub issue: [#1002](https://github.com/dennisonbertram/go-code/issues/1002) +- Parent epic: [#1000](https://github.com/dennisonbertram/go-code/issues/1000) +- Dependency: [#1001](https://github.com/dennisonbertram/go-code/issues/1001), closed and verified as an ancestor of `origin/main` at `fedcf6073135deb7cce1fa49921aa698a9cc7cd7`. +- Repair provenance: local branch `codex/issue-1002-repair-v2` at base + `4cfa5b63e8f1857cf82b5c5000c5d8d8e47e09e0`; the acceptance-audit repair is + locally review-clear. Its promotion candidate is rebased onto `origin/main` + `3506e01c997231c46920b45bf8947c50087dd863`; exact-tree full regression and + live same-conversation CRUD/fire evidence are green, and the guarded PR + update remains pending. +- Problem: the deferred model-facing catalog exposed shell-only creation and lacked a safe in-place update path. An agent could schedule a shell command, but could not create a typed harness continuation or safely edit a recurring job. +- User impact: an operator can ask the agent to create, inspect, change, pause/resume, and delete a recurring conversation job while preserving its stable job ID, immutable scope, execution history, and same-conversation harness behavior. + +## Scope + +- In scope: model-facing shell-compatible and explicit harness `cron_create`, typed prompt execution config, immutable RunMetadata binding at the shared model-registry constructor, ID-only model CRUD/history, distinct ambiguity-safe operator name lookup, scoped SQLite identity and semantic index-metadata migration, atomic store/scheduler reconciliation, mandatory update/pause/resume/delete version tokens, timeout validation, lifecycle/concurrency tests, registration, descriptions, and required documentation/log/index updates. +- Out of scope: raw `cronsd` authentication and readiness (#1003), terminal linkage/overlap (#1004), callback persistence/retry, macOS controls, and any other epic child. + +## Documentation Contract + +- Feature status: `acceptance-audit repair implemented locally, review-clear, rebased, full-regression green, and real-provider same-conversation CRUD/fire green; guarded push, hosted checks, and merge remain pending` +- Public docs affected: embedded tool description and plan/log artifacts; no user guide route list is changed because this is a model-facing tool. +- Spec docs to update before code: this plan and impact map. +- Implementation notes to add after code: engineering, observational, and system logs with exact red/green/full-gate evidence. + +## Test Plan (TDD) + +- New failing tests first: + - `TestCronCreateHarnessJobUsesImmutableRunScope` proves typed harness config and scope override rejection. + - `TestIntegrationCronUpdateCompareAndSwapAllowsOneConcurrentWriter` proves atomic one-winner persistence and no stale active scheduler resurrection. + - `TestServerUpdateJobRejectsNonPositiveTimeout` proves authoritative unsafe-timeout rejection. + - `TestEmbeddedCronModelToolsFullScopedLifecycle` proves create/list/get/update/pause/resume/delete through model tools and a stateful scoped adapter. + - ID-only schema/behavior tests prove model CRUD/history never falls back to names; operator lookup uses a distinct route and returns typed ambiguity. + - legacy SQLite migration variants prove quoted identifier recognition, durable job/history/timestamp preservation, idempotence, scoped uniqueness, and integrity. + - remote owned lifecycle and concurrent update/delete tests prove scoped CRUD/history and no post-delete re-arm under normal/race execution. + - registry, comprehensive tool-list, schema, and embedded-description tests prove catalog coverage. + - assembled default-registry tests prove raw embedded and remote adapters are + scoped automatically from RunMetadata, operator access remains raw, and + stale pause/resume/delete requests conflict without a manual wrapper. + - SQLite `index_list`/`index_xinfo` tests prove single-column collated global + uniqueness is migrated while composite and partial indexes are ignored. + - Remote and embedded lifecycle tests inject scheduler prepare/CAS failure + and restart; create/resume remain paused-first on failure, while active + schedule replacement preserves the old active row/entry until + `Prepare` → CAS → infallible `Commit` succeeds. + - `cron_get` distinguishes a successful empty history from an unavailable + history query while preserving the readable job and array result shape. +- Existing tests to update: cron registry/tool lists, description manifest, remote adapter request mapping, and the assembled embedded harness path. +- Regression tests required: invalid JSON, missing ID/version, invalid timestamp/timeout, client/service errors, omitted-field preservation, concurrent CAS, scope isolation, and harness starter correlation. + +## Cross-Surface Impact Map + +See `2026-07-31-issue-1002-conversational-cron-crud-impact-map.md`. + +## Implementation Checklist + +- [x] Dependency readiness checked against current `origin/main`. +- [x] Current ownership and duplicate-tool search recorded. +- [x] Plan and impact map created before implementation. +- [x] Add failing tests and capture the expected red result. +- [x] Implement atomic persistence CAS and move scheduler reconciliation after a successful write. +- [x] Add explicit model-facing harness creation while preserving legacy shell creation. +- [x] Enforce authoritative non-empty shell commands, non-empty harness prompts, valid schedules, and positive explicitly supplied timeouts on create/update boundaries. +- [x] Replace the test-only scope wrapper with the production model-facing + client boundary; fail closed for cross-scope read/history/mutation across + embedded and remote clients. +- [x] Expose typed harness prompt updates, enforce mutually exclusive execution + inputs, and prove the updated prompt reaches the assembled harness starter. +- [x] Make model CRUD/history ID-only and separate operator name lookup with a + typed ambiguity result. +- [x] Move remote and embedded ownership checks into authoritative SQLite scope + predicates; allow the same name across independent ownership tuples. +- [x] Make active schedule replacement collision-safe with inert `Prepare` → + durable CAS → infallible `Commit`; abort prepare/CAS failure without changing + the prior active durable row or live entry, while create/resume remain + paused-first. +- [x] Migrate inline, named, quoted, bracketed, and backtick global name + constraints without losing jobs or execution history. +- [x] Scope cron once at `NewDefaultRegistryWithOptions`, covering top-level, + worktree per-run, and subagent registries while leaving operator adapters raw; + make an already-scoped client idempotent at that boundary. +- [x] Require `expected_updated_at` from `cron_get` for model pause/resume and, + after auditing the remaining mutations, delete; propagate CAS conflicts + through embedded and remote adapters while preserving raw operator delete. +- [x] Replace SQL-text uniqueness detection with semantic SQLite + `index_list`/`index_xinfo` inspection, including `COLLATE NOCASE`, exact + single-key-column recognition, composite/partial exclusion, idempotence, and + integrity checks. +- [x] Blocking-review affected packages passed normal/race with + `go test [ -race ] ./internal/cron ./internal/harness/tools/... ./internal/harness ./cmd/harnessd -count=1`; normal timings were + 9.284s/11.486s/1.553s/9.323s/1.295s/1.573s/4.473s/6.469s/3.333s and race timings were + 10.958s/12.295s/2.499s/10.693s/1.971s/2.268s/4.424s/6.985s/3.122s in command output order. +- [x] Lifecycle-convergence follow-up passed the same bounded nine-package + normal/race gate. Normal timings were + 8.993s/11.698s/1.642s/9.359s/1.307s/1.547s/4.757s/5.893s/3.169s; race + timings were + 11.223s/12.901s/2.103s/10.298s/1.416s/2.454s/4.639s/7.720s/4.077s. +- [x] Run focused affected-package normal/race: normal passed in + 8.806s/8.565s/1.451s and race passed in 10.886s/10.070s/2.963s for + `internal/cron`, deferred tools, and `cmd/harnessd` respectively. +- [x] Review follow-up: move operator name lookup from a decoded path segment + to a query value; slash, spaces, percent, Unicode, empty input, and method + behavior pass focused normal/race, with the complete cron package green in + 8.783s/10.807s. +- [x] Prior PR candidate passed focused normal/race verification and the + foreground `./scripts/test-regression.sh` at + `coveragegate: PASS (total=85.6%, min=80.0%, zero-functions=0)`; this is + historical evidence only and does not prove the current uncommitted repair. +- [x] Latest admission-lock and history-availability candidate passes + `go test [ -race ] ./internal/cron ./internal/harness ./internal/harness/tools ./internal/harness/tools/deferred ./cmd/harnessd -count=1`. + Normal timings were 9.522s/5.596s/11.403s/9.466s/2.402s; race timings were + 11.202s/8.741s/12.588s/10.954s/4.512s. +- [x] Rebase the repaired candidate onto current `origin/main` at `3506e01c`; + all production/test changes applied cleanly and shared log/index conflicts + retained both histories. +- [x] Run the rebased full foreground regression: normal, complete race, and + coverage pass at 85.7% total with zero uncovered production functions. +- [x] Run a real OpenAI-backed same-conversation canary. Eleven completed runs + exercised all eight model cron tools, rejected a stale update version, + produced two scheduler-started continuations in the original conversation, + preserved linked execution run IDs, and ended with an empty list plus 404 + direct read after deletion. +- [x] Repair the final intent-level review finding: the recurring-cron + description now routes one-shot delayed conversation work to + `set_delayed_callback` instead of `bash`/`sleep`; the red/green regression and + independent cheap-agent re-review are clear. +- [ ] Guarded-force update PR #1057 (`Closes #1002`) and merge only after hosted + checks and the production merge gate pass. + +## Risks and Mitigations + +- Risk: a model mutation could overwrite or delete after a concurrent operator edit. Mitigation: `cron_update`, `cron_pause`, `cron_resume`, and `cron_delete` require `expected_updated_at` from `cron_get`; the service performs persistence-level compare-and-swap and returns typed/HTTP 409 on zero-row matches. Raw operator endpoints retain their existing compatibility contract. +- Risk: omitted JSON fields could erase existing configuration. Mitigation: pointer fields plus tests that assert nil for omitted values. +- Risk: ownership could become model-mutable. Mitigation: update request has no tenant, agent, conversation, or job-ID mutation fields; existing #1001 scope binding remains authoritative. +- Risk: harness creation could silently degrade to shell. Mitigation: explicit `execution_type` validation, typed `{prompt}` config, distinct shell/harness inputs, and an assembled starter test; remote cronsd transport remains #1003. +- Risk: malformed execution config or an unsafe timeout could reach persistence. Mitigation: `ValidateExecutionConfig` runs at HTTP and embedded create/update boundaries; explicit HTTP/tool timeout values must be positive while omitted create values retain the 30-second default. +- Risk: a name-only operator call can match several scoped jobs. Mitigation: + `/v1/jobs/{id}` is ID-only; `/v1/jobs/by-name?name=...` is explicit and returns + typed `ErrJobAmbiguous` unless ownership scope selects one row. +- Risk: scheduler replacement or a queued stale callback could diverge from a + completed lifecycle mutation. Mitigation: active replacement uses inert + `Prepare` → durable CAS → infallible `Commit`, while create/resume remain + paused-first. Globally monotonic registration identities are checked after + jitter and atomically with execution-row creation under the same lock used by + prepare/commit/remove. diff --git a/docs/plans/INDEX.md b/docs/plans/INDEX.md index d3b799f6..a9c846e8 100644 --- a/docs/plans/INDEX.md +++ b/docs/plans/INDEX.md @@ -7,6 +7,9 @@ - `2026-08-01-issue-1081-keychain-parser-coverage-impact-map.md` — Cross-surface impact map for Issue #1081's test-only coverage repair. - `2026-07-31-issue-1056-terminal-assistant-message-plan.md` — Issue #1056 TUI terminal assistant-message reconciliation plan. - `2026-07-31-issue-1056-terminal-assistant-message-impact-map.md` — Cross-surface impact map for Issue #1056. + +- `2026-07-31-issue-1002-conversational-cron-crud-plan.md` — Issue #1002 ID-only, ownership-scoped conversational cron CRUD/history and scheduler consistency repair. +- `2026-07-31-issue-1002-conversational-cron-crud-impact-map.md` — Cross-surface map for Issue #1002 tools, HTTP, SQLite migration, scheduler lifecycle, and compatibility. - `2026-07-31-issue-1077-d0-logo-plan.md` — Issue #1077 D0 macOS app logo geometry, shared SwiftUI source, and verification status. - `2026-07-31-issue-1077-d0-logo-impact-map.md` — Cross-surface impact map for Issue #1077. - `2026-07-31-issue-1076-workflow-initial-write-exit-plan.md` — Issue #1076 plan for preserving child exit diagnostics across the initial workflow write. diff --git a/docs/plans/active-plan.md b/docs/plans/active-plan.md index 0adc8f24..3fc8e791 100644 --- a/docs/plans/active-plan.md +++ b/docs/plans/active-plan.md @@ -12,6 +12,22 @@ and the authoritative foreground full regression are green at 85.7% with zero uncovered production functions. Exact-head review, hosted Ubuntu proof, and parent promotion remain pending. +Current status: Issue #1002 conversational cron CRUD acceptance repair is +implemented locally, review-clear, and rebased onto current `origin/main` +`3506e01c` on `codex/issue-1002-repair-v2`. Model +CRUD/history is ID-only, operator name lookup is ambiguity-safe, and ownership +is enforced in persistence predicates. Create/resume are paused-first; active +replacement is inert `Prepare` → durable CAS → infallible `Commit`. Monotonic +registration identities are rechecked at a scheduler-locked execution admission +point, so a completed pause/delete/replacement cannot be followed by a stale +execution row. Legacy scoped-name migration has durable history/integrity proof. +The exact rebased candidate passes focused normal/race and the complete +foreground regression at 85.7% coverage with zero uncovered functions. A real +OpenAI-backed 11-run conversation proved model create/get/update/list/history/ +pause/resume/delete, stale-CAS rejection, two scheduled same-conversation +continuations visible in SSE and transcript, and final deletion (empty list and +404 read). Guarded PR update, hosted checks, and production merge remain. + Current status: Issue #1067 terminal publication atomicity is implemented on isolated branch `codex/issue-1067-terminal-status-event-atomicity`. Post-review hardening now defines and tests the one-way durable failure contract, bounded diff --git a/internal/cron/cas_integration_test.go b/internal/cron/cas_integration_test.go new file mode 100644 index 00000000..923c8440 --- /dev/null +++ b/internal/cron/cas_integration_test.go @@ -0,0 +1,124 @@ +package cron + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "path/filepath" + "strings" + "sync" + "testing" + "time" +) + +func TestIntegrationCronUpdateCompareAndSwapAllowsOneConcurrentWriter(t *testing.T) { + _, scheduler, client := newIntegrationCronClient(t) + defer scheduler.Stop() + + job, err := client.CreateJob(context.Background(), CreateJobRequest{ + TenantID: "tenant-a", + ConversationID: "conversation-a", + AgentID: "agent-a", + Name: "cas-job", + Schedule: "0 0 * * *", + ExecType: ExecTypeShell, + ExecConfig: `{"command":"echo one"}`, + TimeoutSec: 30, + }) + if err != nil { + t.Fatalf("create: %v", err) + } + + start := make(chan struct{}) + results := make(chan error, 2) + var wg sync.WaitGroup + for _, tag := range []string{"first", "second"} { + tag := tag + wg.Add(1) + go func() { + defer wg.Done() + <-start + _, err := client.UpdateJob(context.Background(), job.ID, UpdateJobRequest{ + Tags: &tag, + ExpectedUpdatedAt: &job.UpdatedAt, + }) + results <- err + }() + } + close(start) + wg.Wait() + close(results) + + var successes, conflicts int + for err := range results { + switch { + case err == nil: + successes++ + case IsJobConflict(err): + conflicts++ + default: + t.Fatalf("unexpected concurrent update error: %v", err) + } + } + if successes != 1 || conflicts != 1 { + t.Fatalf("concurrent update results = successes %d, conflicts %d; want one of each", successes, conflicts) + } + + final, err := client.GetJob(context.Background(), job.ID) + if err != nil { + t.Fatalf("get final job: %v", err) + } + if final.Status != StatusActive { + t.Fatalf("final status = %q, want active; stale PATCH must not restore a prior status", final.Status) + } + if final.Tags != "first" && final.Tags != "second" { + t.Fatalf("final tags = %q, want one winning writer", final.Tags) + } + if !scheduler.HasEntry(job.ID) { + t.Fatal("winning active update must leave the job scheduled") + } + +} + +func newIntegrationCronClient(t *testing.T) (*SQLiteStore, *Scheduler, *Client) { + t.Helper() + store, err := NewSQLiteStore(filepath.Join(t.TempDir(), "cron.db")) + if err != nil { + t.Fatalf("open store: %v", err) + } + if err := store.Migrate(context.Background()); err != nil { + store.Close() + t.Fatalf("migrate: %v", err) + } + scheduler := NewScheduler(store, &ShellExecutor{}, RealClock{}, SchedulerConfig{MaxConcurrent: 2}) + if err := scheduler.Start(context.Background()); err != nil { + store.Close() + t.Fatalf("start scheduler: %v", err) + } + t.Cleanup(func() { store.Close() }) + handler := NewServer(store, scheduler, RealClock{}) + server := httptest.NewServer(handler) + t.Cleanup(server.Close) + return store, scheduler, NewClient(server.URL) +} + +func TestServerUpdateJobRejectsNonPositiveTimeout(t *testing.T) { + store := &mockStore{} + clock := newMockClock(time.Date(2026, 7, 31, 1, 0, 0, 0, time.UTC)) + scheduler := NewScheduler(store, &mockExecutor{}, clock, SchedulerConfig{MaxConcurrent: 1}) + handler := NewServer(store, scheduler, clock) + job := testJob("invalid-timeout") + store.GetJobFunc = func(context.Context, string) (Job, error) { return job, nil } + + for _, timeout := range []int{0, -1} { + t.Run(fmt.Sprintf("timeout_%d", timeout), func(t *testing.T) { + req := httptest.NewRequest(http.MethodPatch, "/v1/jobs/"+job.ID, strings.NewReader(fmt.Sprintf(`{"timeout_seconds":%d}`, timeout))) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusBadRequest || !strings.Contains(rec.Body.String(), "timeout_seconds") { + t.Fatalf("timeout %d response = %d %s, want actionable 400", timeout, rec.Code, rec.Body.String()) + } + }) + } +} diff --git a/internal/cron/client.go b/internal/cron/client.go index 513fe9df..5ee215c2 100644 --- a/internal/cron/client.go +++ b/internal/cron/client.go @@ -7,6 +7,8 @@ import ( "fmt" "io" "net/http" + "net/url" + "time" ) // Client is an HTTP client for the cronsd API. @@ -15,6 +17,41 @@ type Client struct { httpClient *http.Client } +// GetJobByName performs the distinct operator lookup. Model-facing callers use +// GetJob, whose route is ID-only. +func (c *Client) GetJobByName(ctx context.Context, name string) (Job, error) { + if name == "" { + return Job{}, fmt.Errorf("name is required") + } + query := url.Values{"name": []string{name}} + httpReq, err := http.NewRequestWithContext(ctx, http.MethodGet, c.baseURL+"/v1/jobs/by-name?"+query.Encode(), nil) + if err != nil { + return Job{}, fmt.Errorf("create request: %w", err) + } + withScopeHeaders(httpReq) + resp, err := c.httpClient.Do(httpReq) + if err != nil { + return Job{}, fmt.Errorf("do request: %w", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return Job{}, c.parseError(resp) + } + var job Job + if err := json.NewDecoder(resp.Body).Decode(&job); err != nil { + return Job{}, fmt.Errorf("decode response: %w", err) + } + return job, nil +} + +func withScopeHeaders(req *http.Request) { + if scope, ok := ScopeFromContext(req.Context()); ok { + req.Header.Set("X-Cron-Tenant-ID", scope.TenantID) + req.Header.Set("X-Cron-Conversation-ID", scope.ConversationID) + req.Header.Set("X-Cron-Agent-ID", scope.AgentID) + } +} + // NewClient creates a new Client for the given base URL. func NewClient(baseURL string) *Client { return &Client{ @@ -34,6 +71,7 @@ func (c *Client) CreateJob(ctx context.Context, req CreateJobRequest) (Job, erro return Job{}, fmt.Errorf("create request: %w", err) } httpReq.Header.Set("Content-Type", "application/json") + withScopeHeaders(httpReq) resp, err := c.httpClient.Do(httpReq) if err != nil { @@ -58,6 +96,7 @@ func (c *Client) ListJobs(ctx context.Context) ([]Job, error) { if err != nil { return nil, fmt.Errorf("create request: %w", err) } + withScopeHeaders(httpReq) resp, err := c.httpClient.Do(httpReq) if err != nil { @@ -78,12 +117,13 @@ func (c *Client) ListJobs(ctx context.Context) ([]Job, error) { return result.Jobs, nil } -// GetJob retrieves a cron job by ID or name. +// GetJob retrieves a cron job by ID. Operator name lookup is GetJobByName. func (c *Client) GetJob(ctx context.Context, id string) (Job, error) { httpReq, err := http.NewRequestWithContext(ctx, http.MethodGet, c.baseURL+"/v1/jobs/"+id, nil) if err != nil { return Job{}, fmt.Errorf("create request: %w", err) } + withScopeHeaders(httpReq) resp, err := c.httpClient.Do(httpReq) if err != nil { @@ -113,6 +153,7 @@ func (c *Client) UpdateJob(ctx context.Context, id string, req UpdateJobRequest) return Job{}, fmt.Errorf("create request: %w", err) } httpReq.Header.Set("Content-Type", "application/json") + withScopeHeaders(httpReq) resp, err := c.httpClient.Do(httpReq) if err != nil { @@ -133,10 +174,28 @@ func (c *Client) UpdateJob(ctx context.Context, id string, req UpdateJobRequest) // DeleteJob deletes a cron job. func (c *Client) DeleteJob(ctx context.Context, id string) error { - httpReq, err := http.NewRequestWithContext(ctx, http.MethodDelete, c.baseURL+"/v1/jobs/"+id, nil) + return c.deleteJob(ctx, id, nil) +} + +// DeleteJobCAS deletes only when updated_at still matches the version read by +// the caller. DeleteJob remains the unversioned operator API. +func (c *Client) DeleteJobCAS(ctx context.Context, id string, expectedUpdatedAt time.Time) error { + body, err := json.Marshal(DeleteJobRequest{ExpectedUpdatedAt: &expectedUpdatedAt}) + if err != nil { + return fmt.Errorf("marshal request: %w", err) + } + return c.deleteJob(ctx, id, body) +} + +func (c *Client) deleteJob(ctx context.Context, id string, body []byte) error { + httpReq, err := http.NewRequestWithContext(ctx, http.MethodDelete, c.baseURL+"/v1/jobs/"+id, bytes.NewReader(body)) if err != nil { return fmt.Errorf("create request: %w", err) } + if body != nil { + httpReq.Header.Set("Content-Type", "application/json") + } + withScopeHeaders(httpReq) resp, err := c.httpClient.Do(httpReq) if err != nil { @@ -157,6 +216,7 @@ func (c *Client) ListExecutions(ctx context.Context, jobID string, limit, offset if err != nil { return nil, fmt.Errorf("create request: %w", err) } + withScopeHeaders(httpReq) resp, err := c.httpClient.Do(httpReq) if err != nil { @@ -212,6 +272,12 @@ func (c *Client) parseError(resp *http.Response) error { if resp.StatusCode == http.StatusNotFound && errResp.Error.Code == "not_found" { return ErrJobNotFound } + if resp.StatusCode == http.StatusConflict && errResp.Error.Code == "conflict" { + return ErrJobConflict + } + if resp.StatusCode == http.StatusConflict && errResp.Error.Code == "ambiguous" { + return ErrJobAmbiguous + } return fmt.Errorf("HTTP %d: %s: %s", resp.StatusCode, errResp.Error.Code, errResp.Error.Message) } return fmt.Errorf("HTTP %d: %s", resp.StatusCode, string(body)) diff --git a/internal/cron/execution_config.go b/internal/cron/execution_config.go new file mode 100644 index 00000000..cb40de10 --- /dev/null +++ b/internal/cron/execution_config.go @@ -0,0 +1,38 @@ +package cron + +import ( + "encoding/json" + "fmt" + "strings" +) + +// ValidateExecutionConfig rejects executable jobs that cannot safely run. +// The execution type is authoritative: shell jobs require a non-empty command +// and harness jobs require a non-empty prompt. +func ValidateExecutionConfig(execType, execConfig string) error { + switch execType { + case ExecTypeShell, "": + var cfg struct { + Command string `json:"command"` + } + if err := json.Unmarshal([]byte(execConfig), &cfg); err != nil { + return fmt.Errorf("shell execution_config must be valid JSON: %w", err) + } + if strings.TrimSpace(cfg.Command) == "" { + return fmt.Errorf("shell execution_config requires a non-empty command") + } + case ExecTypeHarness: + var cfg struct { + Prompt string `json:"prompt"` + } + if err := json.Unmarshal([]byte(execConfig), &cfg); err != nil { + return fmt.Errorf("harness execution_config must be valid JSON: %w", err) + } + if strings.TrimSpace(cfg.Prompt) == "" { + return fmt.Errorf("harness execution_config requires a non-empty prompt") + } + default: + return fmt.Errorf("execution_type must be shell or harness") + } + return nil +} diff --git a/internal/cron/integration_test.go b/internal/cron/integration_test.go index bd8f48e5..793e0499 100644 --- a/internal/cron/integration_test.go +++ b/internal/cron/integration_test.go @@ -75,7 +75,7 @@ func TestIntegrationCronAPI(t *testing.T) { } // Get job by name. - got, err = client.GetJob(ctx, "echo-hello") + got, err = client.GetJobByName(ctx, "echo-hello") if err != nil { t.Fatalf("get by name: %v", err) } diff --git a/internal/cron/scheduler.go b/internal/cron/scheduler.go index b00ede31..ecd84b77 100644 --- a/internal/cron/scheduler.go +++ b/internal/cron/scheduler.go @@ -13,19 +13,47 @@ import ( // Scheduler manages scheduled jobs using robfig/cron. type Scheduler struct { - store Store - executor Executor - clock Clock - cron *robfigcron.Cron - sem chan struct{} // concurrency semaphore - wg sync.WaitGroup - mu sync.Mutex - entries map[string]robfigcron.EntryID // jobID -> entryID - jitterCfg JitterConfig - jitterCache map[string]time.Duration // jobID|schedule -> jitter offset - sleepFn func(time.Duration) // injectable sleep for testing; defaults to time.Sleep - done chan struct{} // closed by Stop to interrupt in-flight jitter waits - stopOnce sync.Once // guards closing done so a double Stop cannot panic + store Store + executor Executor + clock Clock + cron *robfigcron.Cron + addFunc func(string, func()) (robfigcron.EntryID, error) + sem chan struct{} // concurrency semaphore + wg sync.WaitGroup + mu sync.Mutex + entries map[string]robfigcron.EntryID // jobID -> entryID + generations map[string]uint64 // jobID -> live callback generation + nextGeneration uint64 // never reused, including after pause/delete + prepared map[string]*PreparedJob // jobID -> reserved replacement + jitterCfg JitterConfig + jitterCache map[string]time.Duration // jobID|schedule -> jitter offset + sleepFn func(time.Duration) // injectable sleep for testing; defaults to time.Sleep + done chan struct{} // closed by Stop to interrupt in-flight jitter waits + stopOnce sync.Once // guards closing done so a double Stop cannot panic +} + +// JobScheduler is the live-dispatch subset required by embedded lifecycle +// adapters and their deterministic failure tests. +type JobScheduler interface { + AddJob(Job) error + PrepareJob(Job) (*PreparedJob, error) + CommitJob(*PreparedJob) + AbortJob(*PreparedJob) + UpdateJobSchedule(Job) error + RemoveJob(string) +} + +// PreparedJob is an inert scheduler registration. It cannot dispatch until +// CommitJob makes its generation live; AbortJob removes it without disturbing +// the entry that was live when preparation began. +type PreparedJob struct { + scheduler *Scheduler + jobID string + entryID robfigcron.EntryID + oldEntryID robfigcron.EntryID + hadOld bool + generation uint64 + done bool } // SchedulerConfig holds scheduler configuration. @@ -53,8 +81,11 @@ func NewScheduler(store Store, executor Executor, clock Clock, cfg SchedulerConf executor: executor, clock: clock, cron: c, + addFunc: c.AddFunc, sem: make(chan struct{}, cfg.MaxConcurrent), entries: make(map[string]robfigcron.EntryID), + generations: make(map[string]uint64), + prepared: make(map[string]*PreparedJob), jitterCfg: cfg.Jitter, jitterCache: make(map[string]time.Duration), sleepFn: time.Sleep, @@ -103,6 +134,9 @@ func (s *Scheduler) Stop() { func (s *Scheduler) AddJob(job Job) error { s.mu.Lock() defer s.mu.Unlock() + if s.prepared[job.ID] != nil { + return fmt.Errorf("cron replacement is being prepared for job %s", job.ID) + } // Compute a deterministic jitter offset for this job. jitterCache is // retained (and still populated here, under s.mu) so tests and any @@ -115,17 +149,88 @@ func (s *Scheduler) AddJob(job Job) error { s.jitterCache[jitterCacheKey(job.ID, job.Schedule)] = jitter // Capture job and its jitter offset for the closure. + s.nextGeneration++ + generation := s.nextGeneration j := job - entryID, err := s.cron.AddFunc(job.Schedule, func() { - s.fireJob(j, jitter) + entryID, err := s.addFunc(job.Schedule, func() { + s.fireJobIfCurrent(j, jitter, generation) }) if err != nil { return fmt.Errorf("add cron entry: %w", err) } + // robfig/cron owns the actual registrations. Replacing only our map entry + // would leave the former callback firing in parallel. + if old, ok := s.entries[job.ID]; ok { + s.cron.Remove(old) + } s.entries[job.ID] = entryID + s.generations[job.ID] = generation return nil } +// PrepareJob registers a replacement without making it runnable. The current +// live entry remains untouched until CommitJob, so a scheduler failure cannot +// turn an otherwise healthy active job into a paused durable mutation. +func (s *Scheduler) PrepareJob(job Job) (*PreparedJob, error) { + s.mu.Lock() + defer s.mu.Unlock() + if s.prepared[job.ID] != nil { + return nil, fmt.Errorf("cron replacement is already being prepared for job %s", job.ID) + } + + jitter := computeJitter(s.jitterCfg, job.ID, job.Schedule) + s.jitterCache[jitterCacheKey(job.ID, job.Schedule)] = jitter + s.nextGeneration++ + generation := s.nextGeneration + j := job + entryID, err := s.addFunc(job.Schedule, func() { + s.fireJobIfCurrent(j, jitter, generation) + }) + if err != nil { + return nil, fmt.Errorf("prepare cron entry: %w", err) + } + old, hadOld := s.entries[job.ID] + prepared := &PreparedJob{scheduler: s, jobID: job.ID, entryID: entryID, oldEntryID: old, hadOld: hadOld, generation: generation} + s.prepared[job.ID] = prepared + return prepared, nil +} + +// CommitJob atomically publishes a prepared callback and retires the prior +// one. The generation gate makes an already-queued old callback a no-op. +func (s *Scheduler) CommitJob(prepared *PreparedJob) { + if prepared == nil || prepared.scheduler != s { + return + } + s.mu.Lock() + defer s.mu.Unlock() + if prepared.done { + return + } + s.entries[prepared.jobID] = prepared.entryID + s.generations[prepared.jobID] = prepared.generation + if prepared.hadOld { + s.cron.Remove(prepared.oldEntryID) + } + prepared.done = true + delete(s.prepared, prepared.jobID) +} + +// AbortJob discards an inert prepared entry and leaves the prior live entry +// and its generation unchanged. +func (s *Scheduler) AbortJob(prepared *PreparedJob) { + if prepared == nil || prepared.scheduler != s { + return + } + s.mu.Lock() + defer s.mu.Unlock() + if prepared.done { + return + } + s.cron.Remove(prepared.entryID) + prepared.done = true + delete(s.prepared, prepared.jobID) +} + // HasEntry reports whether jobID is currently registered with the live // cron dispatcher — i.e. it will fire on its schedule. A paused or // removed job is not registered. Exposed primarily so callers (including @@ -161,22 +266,42 @@ func (s *Scheduler) RemoveJob(jobID string) { s.mu.Lock() defer s.mu.Unlock() + if prepared := s.prepared[jobID]; prepared != nil { + s.cron.Remove(prepared.entryID) + prepared.done = true + delete(s.prepared, jobID) + } if entryID, ok := s.entries[jobID]; ok { s.cron.Remove(entryID) delete(s.entries, jobID) } + // Advance the global identity before dropping this per-job entry. A queued + // callback now observes zero (not its old identity), while a later resume + // receives a distinct globally monotonic identity without retaining an + // unbounded tombstone map. + s.nextGeneration++ + delete(s.generations, jobID) } -// UpdateJobSchedule removes the old cron entry and adds a new one. +// UpdateJobSchedule registers the replacement before removing the old entry. +// A malformed replacement therefore cannot unschedule a healthy job. func (s *Scheduler) UpdateJobSchedule(job Job) error { - s.RemoveJob(job.ID) - // Recompute jitter for the new schedule. + prepared, err := s.PrepareJob(job) + if err != nil { + return fmt.Errorf("add replacement cron entry: %w", err) + } + s.CommitJob(prepared) + return nil +} + +func (s *Scheduler) fireJobIfCurrent(job Job, jitter time.Duration, generation uint64) { s.mu.Lock() - s.jitterCache[jitterCacheKey(job.ID, job.Schedule)] = computeJitter( - s.jitterCfg, job.ID, job.Schedule, - ) + live := s.generations[job.ID] == generation s.mu.Unlock() - return s.AddJob(job) + if !live { + return + } + s.fireJobWithScheduleGuard(job, jitter, true, generation) } // fireJob executes a job: creates an execution record, runs the executor, @@ -188,6 +313,10 @@ func (s *Scheduler) UpdateJobSchedule(job Job) error { // the cache concurrently with AddJob's locked write previously caused a // fatal, unrecoverable "concurrent map read and map write" runtime error. func (s *Scheduler) fireJob(job Job, jitter time.Duration) { + s.fireJobWithScheduleGuard(job, jitter, false, 0) +} + +func (s *Scheduler) fireJobWithScheduleGuard(job Job, jitter time.Duration, requireMatchingSchedule bool, generation uint64) { ctx := context.Background() now := s.clock.Now() @@ -238,20 +367,23 @@ func (s *Scheduler) fireJob(job Job, jitter time.Duration) { log.Printf("cron: skipping fire for job %s: no longer active (status=%s)", job.ID, current.Status) return } - job = current - - exec := Execution{ - ID: uuid.New().String(), - JobID: job.ID, - StartedAt: now, - Status: ExecStatusPending, + // A replacement can commit durable state just before it retires the old + // robfig callback. Do not let that stale tick execute the replacement's + // configuration at the old schedule; the new callback owns the new schedule. + if requireMatchingSchedule && current.Schedule != job.Schedule { + log.Printf("cron: skipping stale schedule fire for job %s", job.ID) + return } - - exec, err = s.store.CreateExecution(ctx, exec) + job = current + exec, admitted, err := s.admitExecution(ctx, job, now, generation, requireMatchingSchedule) if err != nil { log.Printf("cron: failed to create execution for job %s: %v", job.ID, err) return } + if !admitted { + log.Printf("cron: skipping stale registration fire for job %s", job.ID) + return + } // Acquire semaphore to limit concurrency. s.sem <- struct{}{} @@ -315,6 +447,30 @@ func (s *Scheduler) fireJob(job Job, jitter time.Duration) { }() } +// admitExecution is the linearization point between a registered callback and +// pause/delete/replacement. Its final identity check and execution-row create +// share s.mu with Prepare/Commit/Remove. The lock is released before executor +// work begins. TriggerJob bypasses the identity guard and keeps its prior path. +func (s *Scheduler) admitExecution(ctx context.Context, job Job, now time.Time, generation uint64, requireCurrent bool) (Execution, bool, error) { + exec := Execution{ + ID: uuid.New().String(), + JobID: job.ID, + StartedAt: now, + Status: ExecStatusPending, + } + if !requireCurrent { + created, err := s.store.CreateExecution(ctx, exec) + return created, true, err + } + s.mu.Lock() + defer s.mu.Unlock() + if s.generations[job.ID] != generation { + return Execution{}, false, nil + } + created, err := s.store.CreateExecution(ctx, exec) + return created, true, err +} + // isTimeoutError checks if an error message indicates a timeout. func isTimeoutError(err error) bool { if err == nil { diff --git a/internal/cron/scheduler_test.go b/internal/cron/scheduler_test.go index 19f80b31..7f9ada3e 100644 --- a/internal/cron/scheduler_test.go +++ b/internal/cron/scheduler_test.go @@ -102,6 +102,23 @@ func TestUpdateJobSchedule(t *testing.T) { } } +// A scheduler entry is a live robfig registration, not merely the entry kept +// in Scheduler.entries. Re-adding an active job must replace the old live +// registration instead of leaking a second future fire. +func TestSchedulerAddJob_ReplacesExistingLiveEntry(t *testing.T) { + s := NewScheduler(&mockStore{}, &mockExecutor{}, RealClock{}, SchedulerConfig{}) + job := testJob("replace-live-entry") + if err := s.AddJob(job); err != nil { + t.Fatalf("first AddJob: %v", err) + } + if err := s.AddJob(job); err != nil { + t.Fatalf("second AddJob: %v", err) + } + if got := len(s.cron.Entries()); got != 1 { + t.Fatalf("live cron entries = %d, want 1", got) + } +} + func TestTriggerJobRejectsMissingOrInactiveJob(t *testing.T) { t.Run("load error", func(t *testing.T) { store := &mockStore{ @@ -1024,6 +1041,145 @@ func TestFireJob_SkipsExecutionWhenJobPausedInStore(t *testing.T) { } } +func TestPreparedReplacementSuppressesCandidateAndStaleCallbacks(t *testing.T) { + old := testJob("prepared-replacement") + updated := old + updated.Schedule = "0 * * * *" + var calls int + store := &mockStore{ + GetJobFunc: func(context.Context, string) (Job, error) { return updated, nil }, + CreateExecutionFunc: func(_ context.Context, exec Execution) (Execution, error) { return exec, nil }, + UpdateExecutionFunc: func(context.Context, Execution) error { return nil }, + TouchJobRunFunc: func(context.Context, string, time.Time, time.Time, time.Time) error { return nil }, + } + executor := &mockExecutor{ExecuteFunc: func(context.Context, Job) (string, error) { calls++; return "ok", nil }} + clock := newMockClock(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + s := NewScheduler(store, executor, clock, SchedulerConfig{MaxConcurrent: 1, Jitter: JitterConfig{Enabled: false}}) + s.sleepFn = func(time.Duration) {} + if err := s.AddJob(old); err != nil { + t.Fatalf("add old: %v", err) + } + prepared, err := s.PrepareJob(updated) + if err != nil { + t.Fatalf("prepare: %v", err) + } + // The prepared candidate is registered with robfig but must be inert until + // commit, even if a tick reaches its callback immediately. + s.fireJobIfCurrent(updated, 0, prepared.generation) + s.wg.Wait() + if calls != 0 { + t.Fatalf("prepared callback executed %d times before commit", calls) + } + s.CommitJob(prepared) + // An old callback queued immediately before commit must not execute the + // replacement configuration on the old schedule. + s.fireJobIfCurrent(old, 0, 1) + s.fireJobIfCurrent(updated, 0, prepared.generation) + s.wg.Wait() + if calls != 1 { + t.Fatalf("callbacks after commit executed %d times, want only current replacement", calls) + } +} + +func TestScheduler_BlockedJitterOldGenerationCannotRunAfterSameSchedulePauseResume(t *testing.T) { + job := testJob("pause-resume-same-schedule") + var mu sync.Mutex + current := job + var executions int + store := &mockStore{ + GetJobFunc: func(context.Context, string) (Job, error) { mu.Lock(); defer mu.Unlock(); return current, nil }, + CreateExecutionFunc: func(_ context.Context, exec Execution) (Execution, error) { + mu.Lock() + executions++ + mu.Unlock() + return exec, nil + }, + UpdateExecutionFunc: func(context.Context, Execution) error { return nil }, + TouchJobRunFunc: func(context.Context, string, time.Time, time.Time, time.Time) error { return nil }, + } + s := NewScheduler(store, &mockExecutor{}, newMockClock(time.Now().UTC()), SchedulerConfig{MaxConcurrent: 1, Jitter: JitterConfig{Enabled: false}}) + started, release := make(chan struct{}), make(chan struct{}) + s.sleepFn = func(time.Duration) { close(started); <-release } + if err := s.AddJob(job); err != nil { + t.Fatalf("add old: %v", err) + } + s.mu.Lock() + oldGeneration := s.generations[job.ID] + s.mu.Unlock() + done := make(chan struct{}) + go func() { s.fireJobIfCurrent(job, time.Minute, oldGeneration); close(done) }() + <-started + // The durable row resumes active with the identical schedule. Only a + // non-reusable registration identity plus a final post-jitter guard can + // distinguish the queued old tick from the new registration. + s.RemoveJob(job.ID) + mu.Lock() + current.Status = StatusPaused + current.Status = StatusActive + mu.Unlock() + if err := s.AddJob(job); err != nil { + t.Fatalf("resume add: %v", err) + } + close(release) + <-done + s.wg.Wait() + mu.Lock() + defer mu.Unlock() + if executions != 0 { + t.Fatalf("queued pre-pause callback created %d executions after resume", executions) + } +} + +func TestScheduler_RemoveWaitsForCurrentCallbackExecutionAdmission(t *testing.T) { + job := testJob("linearized-execution-admission") + createEntered := make(chan struct{}) + releaseCreate := make(chan struct{}) + store := &mockStore{ + GetJobFunc: func(context.Context, string) (Job, error) { return job, nil }, + CreateExecutionFunc: func(_ context.Context, exec Execution) (Execution, error) { + close(createEntered) + <-releaseCreate + return exec, nil + }, + UpdateExecutionFunc: func(context.Context, Execution) error { return nil }, + TouchJobRunFunc: func(context.Context, string, time.Time, time.Time, time.Time) error { return nil }, + } + s := NewScheduler(store, &mockExecutor{}, newMockClock(time.Now().UTC()), SchedulerConfig{MaxConcurrent: 1, Jitter: JitterConfig{Enabled: false}}) + if err := s.AddJob(job); err != nil { + t.Fatalf("add: %v", err) + } + s.mu.Lock() + generation := s.generations[job.ID] + s.mu.Unlock() + fireDone := make(chan struct{}) + go func() { + s.fireJobIfCurrent(job, 0, generation) + s.wg.Wait() + close(fireDone) + }() + <-createEntered + removeStarted := make(chan struct{}) + removeDone := make(chan struct{}) + go func() { + close(removeStarted) + s.RemoveJob(job.ID) + close(removeDone) + }() + <-removeStarted + select { + case <-removeDone: + close(releaseCreate) + <-fireDone + t.Fatal("RemoveJob completed while the current callback was still admitting its execution") + case <-time.After(100 * time.Millisecond): + // Expected: RemoveJob shares the admission critical section and cannot + // linearize before CreateExecution returns. + } + close(releaseCreate) + <-fireDone + <-removeDone +} + // TestFireJob_DoesNotWriteBackStaleFullSnapshot (BT-003, P1) reproduces the // "silently reverts user edits" half of BUG 2: fireJob previously called // store.UpdateJob with the full Job struct it captured at schedule time, diff --git a/internal/cron/server.go b/internal/cron/server.go index acf7318a..9f8aba49 100644 --- a/internal/cron/server.go +++ b/internal/cron/server.go @@ -1,11 +1,14 @@ package cron import ( + "context" "encoding/json" "fmt" + "io" "net/http" "strconv" "strings" + "sync" "time" "github.com/google/uuid" @@ -17,6 +20,7 @@ type Server struct { store Store scheduler *Scheduler clock Clock + mu sync.Mutex } // NewServer creates an http.Handler with cron API routes. @@ -25,6 +29,7 @@ func NewServer(store Store, scheduler *Scheduler, clock Clock) http.Handler { mux := http.NewServeMux() mux.HandleFunc("/healthz", s.handleHealth) mux.HandleFunc("/v1/jobs", s.handleJobs) + mux.HandleFunc("/v1/jobs/by-name", s.handleGetJobByName) mux.HandleFunc("/v1/jobs/", s.handleJobByID) return mux } @@ -45,9 +50,28 @@ func (s *Server) handleJobs(w http.ResponseWriter, r *http.Request) { } func (s *Server) handleCreateJob(w http.ResponseWriter, r *http.Request) { + s.mu.Lock() + defer s.mu.Unlock() r.Body = http.MaxBytesReader(w, r.Body, 1<<20) // 1MB limit + body, err := io.ReadAll(r.Body) + if err != nil { + writeError(w, http.StatusBadRequest, "invalid_json", err.Error()) + return + } var req CreateJobRequest - if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + if err := json.Unmarshal(body, &req); err != nil { + writeError(w, http.StatusBadRequest, "invalid_json", err.Error()) + return + } + if scope, scoped, scopeErr := requestScope(r); scopeErr != nil { + writeError(w, http.StatusBadRequest, "validation_error", scopeErr.Error()) + return + } else if scoped && (req.TenantID != scope.TenantID || req.ConversationID != scope.ConversationID || req.AgentID != scope.AgentID) { + writeError(w, http.StatusForbidden, "forbidden", "cron create scope does not match request scope") + return + } + var rawFields map[string]json.RawMessage + if err := json.Unmarshal(body, &rawFields); err != nil { writeError(w, http.StatusBadRequest, "invalid_json", err.Error()) return } @@ -72,8 +96,16 @@ func (s *Server) handleCreateJob(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "validation_error", "execution_type must be \"shell\" or \"harness\"") return } + if err := ValidateExecutionConfig(req.ExecType, req.ExecConfig); err != nil { + writeError(w, http.StatusBadRequest, "validation_error", err.Error()) + return + } - if req.TimeoutSec <= 0 { + if _, explicitlySet := rawFields["timeout_seconds"]; explicitlySet && req.TimeoutSec <= 0 { + writeError(w, http.StatusBadRequest, "validation_error", "timeout_seconds must be positive") + return + } + if req.TimeoutSec == 0 { req.TimeoutSec = 30 } @@ -87,12 +119,16 @@ func (s *Server) handleCreateJob(w http.ResponseWriter, r *http.Request) { Schedule: req.Schedule, ExecType: req.ExecType, ExecConfig: req.ExecConfig, - Status: StatusActive, - TimeoutSec: req.TimeoutSec, - Tags: req.Tags, - NextRunAt: nextRun, - CreatedAt: now, - UpdatedAt: now, + // Creation is deliberately persisted non-runnable. The live scheduler is + // registered first and this row is CAS-activated only afterwards; a + // scheduler or activation failure can therefore never survive restart as + // an active orphan. + Status: StatusPaused, + TimeoutSec: req.TimeoutSec, + Tags: req.Tags, + NextRunAt: nextRun, + CreatedAt: now, + UpdatedAt: now, } job, err = s.store.CreateJob(r.Context(), job) @@ -101,16 +137,45 @@ func (s *Server) handleCreateJob(w http.ResponseWriter, r *http.Request) { return } - if addErr := s.scheduler.AddJob(job); addErr != nil { + activeJob := job + activeJob.Status = StatusActive + if addErr := s.scheduler.AddJob(activeJob); addErr != nil { + s.scheduler.RemoveJob(job.ID) writeError(w, http.StatusInternalServerError, "scheduler_error", addErr.Error()) return } + activeJob.UpdatedAt = s.clock.Now() + if !activeJob.UpdatedAt.After(job.UpdatedAt) { + activeJob.UpdatedAt = job.UpdatedAt.Add(time.Nanosecond) + } + if err := s.store.UpdateJobCAS(r.Context(), activeJob, job.UpdatedAt); err != nil { + s.scheduler.RemoveJob(job.ID) + writeError(w, http.StatusInternalServerError, "store_error", fmt.Sprintf("activate registered job: %v", err)) + return + } - writeJSON(w, http.StatusCreated, job) + writeJSON(w, http.StatusCreated, activeJob) } func (s *Server) handleListJobs(w http.ResponseWriter, r *http.Request) { - jobs, err := s.store.ListJobs(r.Context()) + scope, scoped, err := requestScope(r) + if err != nil { + writeError(w, http.StatusBadRequest, "validation_error", err.Error()) + return + } + var jobs []Job + if scoped { + if store, ok := s.store.(ScopedStore); ok { + jobs, err = store.ListJobsInScope(r.Context(), scope) + } else { + jobs, err = s.store.ListJobs(r.Context()) + if err == nil { + jobs = filterJobsInScope(jobs, scope) + } + } + } else { + jobs, err = s.store.ListJobs(r.Context()) + } if err != nil { writeError(w, http.StatusInternalServerError, "store_error", err.Error()) return @@ -134,7 +199,6 @@ func (s *Server) handleJobByID(w http.ResponseWriter, r *http.Request) { parts := strings.Split(path, "/") id := parts[0] - if len(parts) == 2 && parts[1] == "history" { s.handleHistory(w, r, id) return @@ -157,28 +221,73 @@ func (s *Server) handleJobByID(w http.ResponseWriter, r *http.Request) { } func (s *Server) handleGetJob(w http.ResponseWriter, r *http.Request, id string) { - job, err := s.store.GetJob(r.Context(), id) + scope, scoped, scopeErr := requestScope(r) + if scopeErr != nil { + writeError(w, http.StatusBadRequest, "validation_error", scopeErr.Error()) + return + } + job, err := s.getJob(r.Context(), id, scope, scoped) if err != nil { - if !IsJobNotFound(err) { + if IsJobNotFound(err) { + writeError(w, http.StatusNotFound, "not_found", "job not found") + } else { writeError(w, http.StatusInternalServerError, "store_error", err.Error()) - return } - // Try by name. - job, err = s.store.GetJobByName(r.Context(), id) - if err != nil { - if !IsJobNotFound(err) { - writeError(w, http.StatusInternalServerError, "store_error", err.Error()) - return - } + return + } + writeJSON(w, http.StatusOK, job) +} + +// handleGetJobByName is a distinct query-parameter operator lookup. A query +// value preserves every non-empty job name, including slashes and percent +// signs, while model-facing CRUD remains on the ID-only /v1/jobs/{id} route. +func (s *Server) handleGetJobByName(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + writeMethodNotAllowed(w, "GET") + return + } + name := r.URL.Query().Get("name") + if name == "" { + writeError(w, http.StatusBadRequest, "validation_error", "name is required") + return + } + scope, scoped, scopeErr := requestScope(r) + if scopeErr != nil { + writeError(w, http.StatusBadRequest, "validation_error", scopeErr.Error()) + return + } + var job Job + var err error + if store, ok := s.store.(ScopedStore); ok && scoped { + job, err = store.GetJobByNameInScope(r.Context(), name, scope) + } else { + job, err = s.store.GetJobByName(r.Context(), name) + if err == nil && scoped && !scope.Matches(job) { + err = ErrJobNotFound + } + } + if err != nil { + if IsJobNotFound(err) { writeError(w, http.StatusNotFound, "not_found", "job not found") - return + } else if IsJobAmbiguous(err) { + writeError(w, http.StatusConflict, "ambiguous", "job name matches multiple scopes; use a job ID or provide scope") + } else { + writeError(w, http.StatusInternalServerError, "store_error", err.Error()) } + return } writeJSON(w, http.StatusOK, job) } func (s *Server) handleUpdateJob(w http.ResponseWriter, r *http.Request, id string) { - job, err := s.store.GetJob(r.Context(), id) + s.mu.Lock() + defer s.mu.Unlock() + scope, scoped, scopeErr := requestScope(r) + if scopeErr != nil { + writeError(w, http.StatusBadRequest, "validation_error", scopeErr.Error()) + return + } + job, err := s.getJob(r.Context(), id, scope, scoped) if err != nil { if !IsJobNotFound(err) { writeError(w, http.StatusInternalServerError, "store_error", err.Error()) @@ -187,6 +296,7 @@ func (s *Server) handleUpdateJob(w http.ResponseWriter, r *http.Request, id stri writeError(w, http.StatusNotFound, "not_found", "job not found") return } + originalJob := job r.Body = http.MaxBytesReader(w, r.Body, 1<<20) // 1MB limit var req UpdateJobRequest @@ -194,6 +304,14 @@ func (s *Server) handleUpdateJob(w http.ResponseWriter, r *http.Request, id stri writeError(w, http.StatusBadRequest, "invalid_json", err.Error()) return } + if req.TimeoutSec != nil && *req.TimeoutSec <= 0 { + writeError(w, http.StatusBadRequest, "validation_error", "timeout_seconds must be positive") + return + } + expectedUpdatedAt := job.UpdatedAt + if req.ExpectedUpdatedAt != nil { + expectedUpdatedAt = req.ExpectedUpdatedAt.UTC() + } if req.Schedule != nil { trimmed := strings.TrimSpace(*req.Schedule) @@ -218,24 +336,17 @@ func (s *Server) handleUpdateJob(w http.ResponseWriter, r *http.Request, id stri if req.Tags != nil { job.Tags = *req.Tags } + if err := ValidateExecutionConfig(job.ExecType, job.ExecConfig); err != nil { + writeError(w, http.StatusBadRequest, "validation_error", err.Error()) + return + } if req.Status != nil { if *req.Status != StatusActive && *req.Status != StatusPaused { writeError(w, http.StatusBadRequest, "validation_error", "status must be \"active\" or \"paused\"") return } - oldStatus := job.Status job.Status = *req.Status - - if *req.Status == StatusPaused && oldStatus != StatusPaused { - s.scheduler.RemoveJob(job.ID) - } - if *req.Status == StatusActive && oldStatus != StatusActive { - if addErr := s.scheduler.AddJob(job); addErr != nil { - writeError(w, http.StatusInternalServerError, "scheduler_error", addErr.Error()) - return - } - } } // Gate on job.Status (the EFFECTIVE post-update status), not on @@ -245,29 +356,97 @@ func (s *Server) handleUpdateJob(w http.ResponseWriter, r *http.Request, id stri // For a resume+schedule PATCH, the status block above already set // job.Status = StatusActive, so this still correctly re-arms // genuinely-active jobs. - if req.Schedule != nil && job.Status == StatusActive { - if err := s.scheduler.UpdateJobSchedule(job); err != nil { + job.UpdatedAt = s.clock.Now() + if !job.UpdatedAt.After(expectedUpdatedAt) { + job.UpdatedAt = expectedUpdatedAt.Add(time.Nanosecond) + } + scheduleChanged := req.Schedule != nil + // Prepare an inert scheduler entry before the durable CAS. A failed prepare + // or CAS leaves the prior active row and its live entry unchanged; after the + // CAS, the in-memory commit is deliberately infallible. + twoPhaseActivate := job.Status == StatusActive && (scheduleChanged || originalJob.Status != StatusActive) + if twoPhaseActivate { + prepared, err := s.scheduler.PrepareJob(job) + if err != nil { writeError(w, http.StatusInternalServerError, "scheduler_error", err.Error()) return } + if err := s.store.UpdateJobCAS(r.Context(), job, expectedUpdatedAt); err != nil { + s.scheduler.AbortJob(prepared) + if IsJobConflict(err) { + writeError(w, http.StatusConflict, "conflict", "cron job changed; refresh before updating") + return + } + writeError(w, http.StatusInternalServerError, "store_error", err.Error()) + return + } + s.scheduler.CommitJob(prepared) + writeJSON(w, http.StatusOK, job) + return } - job.UpdatedAt = s.clock.Now() - if err := s.store.UpdateJob(r.Context(), job); err != nil { + if err := s.store.UpdateJobCAS(r.Context(), job, expectedUpdatedAt); err != nil { + if IsJobConflict(err) { + writeError(w, http.StatusConflict, "conflict", "cron job changed; refresh before updating") + return + } writeError(w, http.StatusInternalServerError, "store_error", err.Error()) return } + if job.Status == StatusPaused { + s.scheduler.RemoveJob(job.ID) + } + writeJSON(w, http.StatusOK, job) } func (s *Server) handleDeleteJob(w http.ResponseWriter, r *http.Request, id string) { - if err := s.store.DeleteJob(r.Context(), id); err != nil { + s.mu.Lock() + defer s.mu.Unlock() + scope, scoped, scopeErr := requestScope(r) + if scopeErr != nil { + writeError(w, http.StatusBadRequest, "validation_error", scopeErr.Error()) + return + } + if _, err := s.getJob(r.Context(), id, scope, scoped); err != nil { if IsJobNotFound(err) { writeError(w, http.StatusNotFound, "not_found", "job not found") + } else { + writeError(w, http.StatusInternalServerError, "store_error", err.Error()) + } + return + } + var req DeleteJobRequest + if r.Body != nil && r.Body != http.NoBody { + body, err := io.ReadAll(r.Body) + if err != nil { + writeError(w, http.StatusBadRequest, "invalid_json", err.Error()) return } - writeError(w, http.StatusInternalServerError, "store_error", err.Error()) + if len(strings.TrimSpace(string(body))) > 0 { + if err := json.Unmarshal(body, &req); err != nil { + writeError(w, http.StatusBadRequest, "invalid_json", err.Error()) + return + } + } + } + var deleteErr error + if req.ExpectedUpdatedAt != nil { + deleteErr = s.store.DeleteJobCAS(r.Context(), id, req.ExpectedUpdatedAt.UTC()) + } else { + deleteErr = s.store.DeleteJob(r.Context(), id) + } + if deleteErr != nil { + if IsJobNotFound(deleteErr) { + writeError(w, http.StatusNotFound, "not_found", "job not found") + return + } + if IsJobConflict(deleteErr) { + writeError(w, http.StatusConflict, "conflict", "cron job changed; call cron_get and retry") + return + } + writeError(w, http.StatusInternalServerError, "store_error", deleteErr.Error()) return } s.scheduler.RemoveJob(id) @@ -279,6 +458,19 @@ func (s *Server) handleHistory(w http.ResponseWriter, r *http.Request, jobID str writeMethodNotAllowed(w, "GET") return } + scope, scoped, scopeErr := requestScope(r) + if scopeErr != nil { + writeError(w, http.StatusBadRequest, "validation_error", scopeErr.Error()) + return + } + if _, err := s.getJob(r.Context(), jobID, scope, scoped); err != nil { + if IsJobNotFound(err) { + writeError(w, http.StatusNotFound, "not_found", "job not found") + } else { + writeError(w, http.StatusInternalServerError, "store_error", err.Error()) + } + return + } limit := 20 offset := 0 @@ -304,6 +496,38 @@ func (s *Server) handleHistory(w http.ResponseWriter, r *http.Request, jobID str writeJSON(w, http.StatusOK, map[string]any{"executions": execs}) } +func requestScope(r *http.Request) (Scope, bool, error) { + scope := Scope{TenantID: r.Header.Get("X-Cron-Tenant-ID"), ConversationID: r.Header.Get("X-Cron-Conversation-ID"), AgentID: r.Header.Get("X-Cron-Agent-ID")} + if scope.TenantID == "" && scope.ConversationID == "" && scope.AgentID == "" { + return Scope{}, false, nil + } + if !scope.Complete() { + return Scope{}, false, fmt.Errorf("complete cron scope is required") + } + return scope, true, nil +} + +func filterJobsInScope(jobs []Job, scope Scope) []Job { + filtered := make([]Job, 0, len(jobs)) + for _, job := range jobs { + if scope.Matches(job) { + filtered = append(filtered, job) + } + } + return filtered +} + +func (s *Server) getJob(ctx context.Context, id string, scope Scope, scoped bool) (Job, error) { + if store, ok := s.store.(ScopedStore); ok && scoped { + return store.GetJobInScope(ctx, id, scope) + } + job, err := s.store.GetJob(ctx, id) + if err == nil && scoped && !scope.Matches(job) { + return Job{}, ErrJobNotFound + } + return job, err +} + func NextRunTime(schedule string, from time.Time) (time.Time, error) { parser := robfigcron.NewParser(robfigcron.Minute | robfigcron.Hour | robfigcron.Dom | robfigcron.Month | robfigcron.Dow) sched, err := parser.Parse(schedule) diff --git a/internal/cron/server_test.go b/internal/cron/server_test.go index 208366d4..60ffe440 100644 --- a/internal/cron/server_test.go +++ b/internal/cron/server_test.go @@ -14,6 +14,8 @@ import ( "sync/atomic" "testing" "time" + + robfigcron "github.com/robfig/cron/v3" ) func newTestServer(t *testing.T) (http.Handler, *mockStore) { @@ -44,6 +46,507 @@ func TestServerHealth(t *testing.T) { } } +func TestServerListJobsFallbackFiltersCompleteScope(t *testing.T) { + scope := Scope{TenantID: "tenant-a", ConversationID: "conversation-a", AgentID: "agent-a"} + owned := testJob("owned") + owned.TenantID, owned.ConversationID, owned.AgentID = scope.TenantID, scope.ConversationID, scope.AgentID + otherTenant := testJob("other-tenant") + otherTenant.TenantID, otherTenant.ConversationID, otherTenant.AgentID = "tenant-b", scope.ConversationID, scope.AgentID + otherConversation := testJob("other-conversation") + otherConversation.TenantID, otherConversation.ConversationID, otherConversation.AgentID = scope.TenantID, "conversation-b", scope.AgentID + otherAgent := testJob("other-agent") + otherAgent.TenantID, otherAgent.ConversationID, otherAgent.AgentID = scope.TenantID, scope.ConversationID, "agent-b" + + // mockStore deliberately implements Store but not ScopedStore. This proves + // the compatibility fallback applies the complete ownership tuple instead + // of returning an unfiltered cross-conversation list. + store := &mockStore{ListJobsFunc: func(context.Context) ([]Job, error) { + return []Job{owned, otherTenant, otherConversation, otherAgent}, nil + }} + clock := newMockClock(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + handler := NewServer(store, NewScheduler(store, &mockExecutor{}, clock, SchedulerConfig{}), clock) + req := httptest.NewRequest(http.MethodGet, "/v1/jobs", nil) + req.Header.Set("X-Cron-Tenant-ID", scope.TenantID) + req.Header.Set("X-Cron-Conversation-ID", scope.ConversationID) + req.Header.Set("X-Cron-Agent-ID", scope.AgentID) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("scoped fallback list status = %d, body=%s", w.Code, w.Body.String()) + } + var payload struct { + Jobs []Job `json:"jobs"` + } + if err := json.NewDecoder(w.Body).Decode(&payload); err != nil { + t.Fatalf("decode scoped fallback list: %v", err) + } + if len(payload.Jobs) != 1 || payload.Jobs[0].ID != owned.ID { + t.Fatalf("scoped fallback list = %#v, want only %s", payload.Jobs, owned.ID) + } +} + +func TestRemoteClient_ScopeIsolatesCRUDAndHistory(t *testing.T) { + store := newTestStore(t) + clock := newMockClock(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + scheduler := NewScheduler(store, &mockExecutor{}, clock, SchedulerConfig{}) + ts := httptest.NewServer(NewServer(store, scheduler, clock)) + t.Cleanup(ts.Close) + client := NewClient(ts.URL) + scopeA := Scope{TenantID: "tenant-a", ConversationID: "conversation", AgentID: "agent"} + scopeB := Scope{TenantID: "tenant-b", ConversationID: "conversation", AgentID: "agent"} + create := func(scope Scope) Job { + job, err := client.CreateJob(WithScope(context.Background(), scope), CreateJobRequest{TenantID: scope.TenantID, ConversationID: scope.ConversationID, AgentID: scope.AgentID, Name: "same-name", Schedule: "*/5 * * * *", ExecType: ExecTypeShell, ExecConfig: `{"command":"echo ok"}`}) + if err != nil { + t.Fatalf("create %s: %v", scope.TenantID, err) + } + return job + } + jobA, jobB := create(scopeA), create(scopeB) + if jobs, err := client.ListJobs(WithScope(context.Background(), scopeA)); err != nil || len(jobs) != 1 || jobs[0].ID != jobA.ID { + t.Fatalf("scoped list = %#v, %v", jobs, err) + } + if _, err := client.GetJob(WithScope(context.Background(), scopeA), jobB.ID); !IsJobNotFound(err) { + t.Fatalf("cross-scope get error = %v, want not found", err) + } + tags := "blocked" + if _, err := client.UpdateJob(WithScope(context.Background(), scopeA), jobB.ID, UpdateJobRequest{Tags: &tags}); !IsJobNotFound(err) { + t.Fatalf("cross-scope update error = %v, want not found", err) + } + if err := client.DeleteJob(WithScope(context.Background(), scopeA), jobB.ID); !IsJobNotFound(err) { + t.Fatalf("cross-scope delete error = %v, want not found", err) + } + if _, err := store.CreateExecution(context.Background(), Execution{ID: "exec-b", JobID: jobB.ID, StartedAt: clock.Now(), Status: ExecStatusSuccess}); err != nil { + t.Fatalf("create execution: %v", err) + } + if _, err := client.ListExecutions(WithScope(context.Background(), scopeA), jobB.ID, 10, 0); !IsJobNotFound(err) { + t.Fatalf("cross-scope history error = %v, want not found", err) + } + if _, err := client.GetJob(WithScope(context.Background(), scopeB), "same-name"); !IsJobNotFound(err) { + t.Fatalf("ID-only route accepted a name: %v", err) + } + if got, err := client.GetJobByName(WithScope(context.Background(), scopeB), "same-name"); err != nil || got.ID != jobB.ID { + t.Fatalf("scoped operator name lookup = %#v, %v", got, err) + } + if _, err := client.GetJobByName(context.Background(), "same-name"); !IsJobAmbiguous(err) { + t.Fatalf("global operator name lookup error = %v, want ambiguity", err) + } + + ctxA := WithScope(context.Background(), scopeA) + gotA, err := client.GetJob(ctxA, jobA.ID) + if err != nil || gotA.ID != jobA.ID { + t.Fatalf("owned get = %#v, %v", gotA, err) + } + if _, err := store.CreateExecution(context.Background(), Execution{ID: "exec-a", JobID: jobA.ID, StartedAt: clock.Now(), Status: ExecStatusSuccess, RunID: "run-a"}); err != nil { + t.Fatalf("create owned execution: %v", err) + } + ownedHistory, err := client.ListExecutions(ctxA, jobA.ID, 10, 0) + if err != nil || len(ownedHistory) != 1 || ownedHistory[0].RunID != "run-a" { + t.Fatalf("owned history = %#v, %v", ownedHistory, err) + } + newSchedule, ownedTags := "15 * * * *", "owned-update" + updated, err := client.UpdateJob(ctxA, jobA.ID, UpdateJobRequest{Schedule: &newSchedule, Tags: &ownedTags, ExpectedUpdatedAt: &gotA.UpdatedAt}) + if err != nil || updated.Schedule != newSchedule || updated.Tags != ownedTags || updated.ID != jobA.ID { + t.Fatalf("owned update = %#v, %v", updated, err) + } + paused := StatusPaused + pausedJob, err := client.UpdateJob(ctxA, jobA.ID, UpdateJobRequest{Status: &paused, ExpectedUpdatedAt: &updated.UpdatedAt}) + if err != nil || pausedJob.Status != StatusPaused || scheduler.HasEntry(jobA.ID) { + t.Fatalf("owned pause = %#v, entry=%v, err=%v", pausedJob, scheduler.HasEntry(jobA.ID), err) + } + active := StatusActive + resumed, err := client.UpdateJob(ctxA, jobA.ID, UpdateJobRequest{Status: &active, ExpectedUpdatedAt: &pausedJob.UpdatedAt}) + if err != nil || resumed.Status != StatusActive || !scheduler.HasEntry(jobA.ID) { + t.Fatalf("owned resume = %#v, entry=%v, err=%v", resumed, scheduler.HasEntry(jobA.ID), err) + } + activeAgain, err := client.UpdateJob(ctxA, jobA.ID, UpdateJobRequest{Status: &active, ExpectedUpdatedAt: &resumed.UpdatedAt}) + if err != nil || activeAgain.Status != StatusActive || len(scheduler.cron.Entries()) != 2 { + t.Fatalf("active resume duplicated live entries: job=%#v entries=%d err=%v", activeAgain, len(scheduler.cron.Entries()), err) + } + if err := client.DeleteJob(ctxA, jobA.ID); err != nil { + t.Fatalf("owned delete: %v", err) + } + if scheduler.HasEntry(jobA.ID) { + t.Fatal("owned delete left a live scheduler entry") + } + if _, err := client.GetJob(ctxA, jobA.ID); !IsJobNotFound(err) { + t.Fatalf("owned deleted get error = %v, want not found", err) + } + if gotB, err := client.GetJob(WithScope(context.Background(), scopeB), jobB.ID); err != nil || gotB.ID != jobB.ID { + t.Fatalf("scope B was affected by scope A lifecycle: %#v, %v", gotB, err) + } + if len(scheduler.cron.Entries()) != 1 { + t.Fatalf("live entries after owned delete = %d, want only scope B", len(scheduler.cron.Entries())) + } +} + +func TestRemoteClient_ConcurrentUpdateDeleteNeverRearmsDeletedJob(t *testing.T) { + store := newTestStore(t) + clock := newMockClock(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + scheduler := NewScheduler(store, &mockExecutor{}, clock, SchedulerConfig{}) + ts := httptest.NewServer(NewServer(store, scheduler, clock)) + t.Cleanup(ts.Close) + client := NewClient(ts.URL) + scope := Scope{TenantID: "tenant", ConversationID: "conversation", AgentID: "agent"} + ctx := WithScope(context.Background(), scope) + job, err := client.CreateJob(ctx, CreateJobRequest{TenantID: scope.TenantID, ConversationID: scope.ConversationID, AgentID: scope.AgentID, Name: "concurrent", Schedule: "*/5 * * * *", ExecType: ExecTypeShell, ExecConfig: `{"command":"echo ok"}`}) + if err != nil { + t.Fatalf("create: %v", err) + } + start := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(2) + var updateErr, deleteErr error + go func() { + defer wg.Done() + <-start + schedule := "15 * * * *" + _, updateErr = client.UpdateJob(ctx, job.ID, UpdateJobRequest{Schedule: &schedule, ExpectedUpdatedAt: &job.UpdatedAt}) + }() + go func() { defer wg.Done(); <-start; deleteErr = client.DeleteJob(ctx, job.ID) }() + close(start) + wg.Wait() + if deleteErr != nil { + t.Fatalf("delete: %v", deleteErr) + } + if updateErr != nil && !IsJobNotFound(updateErr) && !IsJobConflict(updateErr) { + t.Fatalf("update: %v", updateErr) + } + if _, err := client.GetJob(ctx, job.ID); !IsJobNotFound(err) { + t.Fatalf("post-delete get = %v, want not found", err) + } + if scheduler.HasEntry(job.ID) || len(scheduler.cron.Entries()) != 0 { + t.Fatalf("deleted job rearmed: has=%v entries=%d", scheduler.HasEntry(job.ID), len(scheduler.cron.Entries())) + } +} + +func TestServerUpdate_SchedulerReplacementFailureRollsBackPersistence(t *testing.T) { + store := newTestStore(t) + clock := newMockClock(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + scheduler := NewScheduler(store, &mockExecutor{}, clock, SchedulerConfig{}) + job := testJob("rollback-scheduler") + job.ID, job.Schedule = "rollback-scheduler", "*/5 * * * *" + if _, err := store.CreateJob(context.Background(), job); err != nil { + t.Fatalf("create job: %v", err) + } + if err := scheduler.AddJob(job); err != nil { + t.Fatalf("arm old job: %v", err) + } + scheduler.addFunc = func(string, func()) (robfigcron.EntryID, error) { return 0, fmt.Errorf("injected add failure") } + handler := NewServer(store, scheduler, clock) + req := httptest.NewRequest(http.MethodPatch, "/v1/jobs/"+job.ID, strings.NewReader(`{"schedule":"0 * * * *"}`)) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + if w.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, body=%s", w.Code, w.Body.String()) + } + persisted, err := store.GetJob(context.Background(), job.ID) + if err != nil { + t.Fatalf("load after failed replacement: %v", err) + } + if persisted != job { + t.Fatalf("persisted after failed replacement = %#v, want exact pre-update %#v", persisted, job) + } + if !scheduler.HasEntry(job.ID) || len(scheduler.cron.Entries()) != 1 { + t.Fatalf("failed replacement must preserve old live entry: has=%v entries=%d", scheduler.HasEntry(job.ID), len(scheduler.cron.Entries())) + } +} + +type updateCASFailingStore struct{ Store } + +func (updateCASFailingStore) UpdateJobCAS(context.Context, Job, time.Time) error { + return fmt.Errorf("injected update CAS failure") +} + +func TestServerUpdate_PreparedReplacementCASFailureAbortsAndPreservesOldJob(t *testing.T) { + baseStore := newTestStore(t) + clock := newMockClock(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + scheduler := NewScheduler(baseStore, &mockExecutor{}, clock, SchedulerConfig{}) + job := testJob("prepared-cas-failure") + job.ID, job.Schedule = "prepared-cas-failure", "*/5 * * * *" + created, err := baseStore.CreateJob(context.Background(), job) + if err != nil { + t.Fatalf("create job: %v", err) + } + if err := scheduler.AddJob(created); err != nil { + t.Fatalf("arm old job: %v", err) + } + handler := NewServer(updateCASFailingStore{Store: baseStore}, scheduler, clock) + w := httptest.NewRecorder() + handler.ServeHTTP(w, httptest.NewRequest(http.MethodPatch, "/v1/jobs/"+created.ID, strings.NewReader(`{"schedule":"0 * * * *"}`))) + if w.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, body=%s", w.Code, w.Body.String()) + } + persisted, err := baseStore.GetJob(context.Background(), created.ID) + if err != nil || persisted != created { + t.Fatalf("persisted after CAS failure = %#v, %v; want exact old %#v", persisted, err, created) + } + if !scheduler.HasEntry(created.ID) || len(scheduler.cron.Entries()) != 1 { + t.Fatalf("CAS failure must abort prepared entry and preserve old live entry: has=%v entries=%d", scheduler.HasEntry(created.ID), len(scheduler.cron.Entries())) + } +} + +func TestServerUpdate_RedundantActiveStatusDoesNotReregister(t *testing.T) { + store := newTestStore(t) + clock := newMockClock(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + scheduler := NewScheduler(store, &mockExecutor{}, clock, SchedulerConfig{}) + job := testJob("redundant-active") + created, err := store.CreateJob(context.Background(), job) + if err != nil { + t.Fatalf("create: %v", err) + } + if err := scheduler.AddJob(created); err != nil { + t.Fatalf("arm: %v", err) + } + adds := 0 + scheduler.addFunc = func(string, func()) (robfigcron.EntryID, error) { adds++; return 0, fmt.Errorf("unexpected add") } + handler := NewServer(store, scheduler, clock) + w := httptest.NewRecorder() + handler.ServeHTTP(w, httptest.NewRequest(http.MethodPatch, "/v1/jobs/"+created.ID, strings.NewReader(`{"status":"active","tags":"changed"}`))) + if w.Code != http.StatusOK { + t.Fatalf("status = %d, body=%s", w.Code, w.Body.String()) + } + if adds != 0 || !scheduler.HasEntry(created.ID) || len(scheduler.cron.Entries()) != 1 { + t.Fatalf("redundant active status mutated scheduler: adds=%d has=%v entries=%d", adds, scheduler.HasEntry(created.ID), len(scheduler.cron.Entries())) + } + persisted, err := store.GetJob(context.Background(), created.ID) + if err != nil || persisted.Tags != "changed" || persisted.Status != StatusActive { + t.Fatalf("persisted redundant-active update = %#v, %v", persisted, err) + } +} + +func TestServerUpdate_SchedulerFailureAndRollbackConflictConvergesFailClosed(t *testing.T) { + store := newTestStore(t) + clock := newMockClock(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + scheduler := NewScheduler(store, &mockExecutor{}, clock, SchedulerConfig{}) + job := testJob("rollback-conflict") + job.ID, job.Schedule = "rollback-conflict", "*/5 * * * *" + created, err := store.CreateJob(context.Background(), job) + if err != nil { + t.Fatalf("create job: %v", err) + } + if err := scheduler.AddJob(created); err != nil { + t.Fatalf("arm old job: %v", err) + } + + var touchOnce sync.Once + scheduler.addFunc = func(string, func()) (robfigcron.EntryID, error) { + touchOnce.Do(func() { + current, getErr := store.GetJob(context.Background(), created.ID) + if getErr != nil { + t.Fatalf("load job before concurrent touch: %v", getErr) + } + if touchErr := store.TouchJobRun( + context.Background(), current.ID, clock.Now(), current.NextRunAt, current.UpdatedAt.Add(time.Second), + ); touchErr != nil { + t.Fatalf("concurrent touch: %v", touchErr) + } + }) + return 0, fmt.Errorf("injected persistent add failure") + } + + handler := NewServer(store, scheduler, clock) + req := httptest.NewRequest(http.MethodPatch, "/v1/jobs/"+created.ID, strings.NewReader(`{"schedule":"0 * * * *"}`)) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + if w.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, body=%s", w.Code, w.Body.String()) + } + persisted, err := store.GetJob(context.Background(), created.ID) + if err != nil { + t.Fatalf("load after failed replacement: %v", err) + } + if persisted.Status != StatusActive || persisted.Schedule != created.Schedule || persisted.ExecConfig != created.ExecConfig { + t.Fatalf("persisted after prepare failure = %#v, want original active config %#v", persisted, created) + } + if persisted.LastRunAt.IsZero() { + t.Fatal("fail-closed convergence overwrote the concurrent TouchJobRun") + } + if !scheduler.HasEntry(created.ID) { + t.Fatal("prepare failure removed the old runnable scheduler entry") + } +} + +func TestServerUpdate_AddFailureAndRollbackConflictConvergesFailClosed(t *testing.T) { + store := newTestStore(t) + clock := newMockClock(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + scheduler := NewScheduler(store, &mockExecutor{}, clock, SchedulerConfig{}) + job := testJob("resume-rollback-conflict") + job.ID, job.Status = "resume-rollback-conflict", StatusPaused + created, err := store.CreateJob(context.Background(), job) + if err != nil { + t.Fatalf("create paused job: %v", err) + } + + var touchOnce sync.Once + scheduler.addFunc = func(string, func()) (robfigcron.EntryID, error) { + touchOnce.Do(func() { + current, getErr := store.GetJob(context.Background(), created.ID) + if getErr != nil { + t.Fatalf("load job before concurrent touch: %v", getErr) + } + if touchErr := store.TouchJobRun( + context.Background(), current.ID, clock.Now(), current.NextRunAt, current.UpdatedAt.Add(time.Second), + ); touchErr != nil { + t.Fatalf("concurrent touch: %v", touchErr) + } + }) + return 0, fmt.Errorf("injected persistent add failure") + } + + handler := NewServer(store, scheduler, clock) + req := httptest.NewRequest(http.MethodPatch, "/v1/jobs/"+created.ID, strings.NewReader(`{"status":"active"}`)) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + if w.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, body=%s", w.Code, w.Body.String()) + } + persisted, err := store.GetJob(context.Background(), created.ID) + if err != nil { + t.Fatalf("load after failed resume: %v", err) + } + if persisted.Status != StatusPaused { + t.Fatalf("persisted status = %q, want fail-closed paused", persisted.Status) + } + if persisted.LastRunAt.IsZero() { + t.Fatal("fail-closed convergence overwrote the concurrent TouchJobRun") + } + if scheduler.HasEntry(created.ID) { + t.Fatal("fail-closed convergence left a runnable scheduler entry") + } +} + +func TestServerUpdate_RollbackConflictReloadsAndRestoresAuthoritativeActiveJob(t *testing.T) { + store := newTestStore(t) + clock := newMockClock(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + scheduler := NewScheduler(store, &mockExecutor{}, clock, SchedulerConfig{}) + job := testJob("reload-active") + job.ID, job.Schedule = "reload-active", "*/5 * * * *" + created, err := store.CreateJob(context.Background(), job) + if err != nil { + t.Fatalf("create job: %v", err) + } + if err := scheduler.AddJob(created); err != nil { + t.Fatalf("arm old job: %v", err) + } + + realAdd := scheduler.addFunc + addAttempts := 0 + scheduler.addFunc = func(spec string, callback func()) (robfigcron.EntryID, error) { + addAttempts++ + if addAttempts == 1 { + current, getErr := store.GetJob(context.Background(), created.ID) + if getErr != nil { + t.Fatalf("load job before concurrent touch: %v", getErr) + } + if touchErr := store.TouchJobRun( + context.Background(), current.ID, clock.Now(), current.NextRunAt, current.UpdatedAt.Add(time.Second), + ); touchErr != nil { + t.Fatalf("concurrent touch: %v", touchErr) + } + return 0, fmt.Errorf("injected one-shot replacement failure") + } + return realAdd(spec, callback) + } + + handler := NewServer(store, scheduler, clock) + req := httptest.NewRequest(http.MethodPatch, "/v1/jobs/"+created.ID, strings.NewReader(`{"schedule":"0 * * * *"}`)) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + if w.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, body=%s", w.Code, w.Body.String()) + } + persisted, err := store.GetJob(context.Background(), created.ID) + if err != nil { + t.Fatalf("load after recovered replacement: %v", err) + } + if persisted.Status != StatusActive || persisted.Schedule != created.Schedule || persisted.ExecConfig != created.ExecConfig { + t.Fatalf("persisted job = %#v, want original active config %#v", persisted, created) + } + if persisted.LastRunAt.IsZero() { + t.Fatal("fail-closed staging overwrote the concurrent TouchJobRun") + } + if !scheduler.HasEntry(created.ID) || len(scheduler.cron.Entries()) != 1 { + t.Fatalf("prepare failure lost old live entry: has=%v entries=%d", scheduler.HasEntry(created.ID), len(scheduler.cron.Entries())) + } + if addAttempts != 1 { + t.Fatalf("add attempts = %d, want one failed prepared registration", addAttempts) + } +} + +type deleteFailingStore struct { + Store + err error +} + +func (s *deleteFailingStore) DeleteJob(context.Context, string) error { return s.err } +func (s *deleteFailingStore) DeactivateJob(context.Context, string) error { return s.err } + +func TestServerCreate_SchedulerAndDeleteFailureLeavesDurablyInactiveJob(t *testing.T) { + baseStore := newTestStore(t) + store := &deleteFailingStore{Store: baseStore, err: fmt.Errorf("injected delete failure")} + clock := newMockClock(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + scheduler := NewScheduler(store, &mockExecutor{}, clock, SchedulerConfig{}) + scheduler.addFunc = func(string, func()) (robfigcron.EntryID, error) { + return 0, fmt.Errorf("injected add failure") + } + handler := NewServer(store, scheduler, clock) + req := httptest.NewRequest(http.MethodPost, "/v1/jobs", strings.NewReader( + `{"name":"create-fail-closed","schedule":"*/5 * * * *","execution_type":"shell","execution_config":"{\"command\":\"echo ok\"}"}`, + )) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + if w.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, body=%s", w.Code, w.Body.String()) + } + jobs, err := baseStore.ListJobs(context.Background()) + if err != nil { + t.Fatalf("list jobs: %v", err) + } + if len(jobs) != 1 { + t.Fatalf("jobs = %d, want one retained fail-closed record", len(jobs)) + } + if jobs[0].Status != StatusPaused { + t.Fatalf("retained status = %q, want paused", jobs[0].Status) + } + if scheduler.HasEntry(jobs[0].ID) { + t.Fatal("failed create left a runnable scheduler entry") + } +} + +type activationFailingStore struct{ Store } + +func (activationFailingStore) UpdateJobCAS(context.Context, Job, time.Time) error { + return fmt.Errorf("injected activation failure") +} + +func TestServerCreate_ActivationFailureNeverRestartRearmsPausedJob(t *testing.T) { + baseStore := newTestStore(t) + clock := newMockClock(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + store := activationFailingStore{Store: baseStore} + scheduler := NewScheduler(store, &mockExecutor{}, clock, SchedulerConfig{}) + handler := NewServer(store, scheduler, clock) + w := httptest.NewRecorder() + handler.ServeHTTP(w, httptest.NewRequest(http.MethodPost, "/v1/jobs", strings.NewReader(`{"name":"activation-fail","schedule":"*/5 * * * *","execution_type":"shell","execution_config":"{\"command\":\"echo ok\"}"}`))) + if w.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, body=%s", w.Code, w.Body.String()) + } + jobs, err := baseStore.ListJobs(context.Background()) + if err != nil || len(jobs) != 1 || jobs[0].Status != StatusPaused { + t.Fatalf("durable create after activation failure = %#v, %v", jobs, err) + } + if scheduler.HasEntry(jobs[0].ID) { + t.Fatal("activation failure left a live entry") + } + restarted := NewScheduler(baseStore, &mockExecutor{}, clock, SchedulerConfig{}) + if err := restarted.Start(context.Background()); err != nil { + t.Fatalf("restart: %v", err) + } + t.Cleanup(restarted.Stop) + if restarted.HasEntry(jobs[0].ID) { + t.Fatal("paused failed create rearmed after restart") + } +} + func TestServerCreateJob(t *testing.T) { handler, store := newTestServer(t) store.CreateJobFunc = func(_ context.Context, job Job) (Job, error) { @@ -103,6 +606,33 @@ func TestServerCreateJobValidation(t *testing.T) { } } +func TestServerCreateJobRejectsUnsafeExecutionConfigAndTimeout(t *testing.T) { + handler, _ := newTestServer(t) + tests := []struct { + name string + payload string + errMsg string + }{ + {"empty shell command", `{"name":"x","schedule":"* * * * *","execution_type":"shell","execution_config":"{\"command\":\"\"}"}`, "non-empty command"}, + {"incomplete harness prompt", `{"name":"x","schedule":"* * * * *","execution_type":"harness","execution_config":"{\"prompt\":\" \"}"}`, "non-empty prompt"}, + {"zero timeout", `{"name":"x","schedule":"* * * * *","execution_type":"shell","execution_config":"{\"command\":\"echo hi\"}","timeout_seconds":0}`, "timeout_seconds must be positive"}, + {"negative timeout", `{"name":"x","schedule":"* * * * *","execution_type":"shell","execution_config":"{\"command\":\"echo hi\"}","timeout_seconds":-1}`, "timeout_seconds must be positive"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/v1/jobs", strings.NewReader(tt.payload)) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), tt.errMsg) { + t.Fatalf("expected error containing %q, got %s", tt.errMsg, w.Body.String()) + } + }) + } +} + func TestServerListJobs(t *testing.T) { handler, store := newTestServer(t) j := testJob("list-test") @@ -158,7 +688,6 @@ func TestServerGetJobByID(t *testing.T) { } return Job{}, sql.ErrNoRows } - req := httptest.NewRequest(http.MethodGet, "/v1/jobs/"+j.ID, nil) w := httptest.NewRecorder() handler.ServeHTTP(w, req) @@ -175,7 +704,7 @@ func TestServerGetJobByID(t *testing.T) { } } -func TestServerGetJobByName(t *testing.T) { +func TestServerOperatorGetJobByNameUsesDistinctRoute(t *testing.T) { handler, store := newTestServer(t) j := testJob("named-job") store.GetJobFunc = func(_ context.Context, id string) (Job, error) { @@ -188,7 +717,7 @@ func TestServerGetJobByName(t *testing.T) { return Job{}, sql.ErrNoRows } - req := httptest.NewRequest(http.MethodGet, "/v1/jobs/named-job", nil) + req := httptest.NewRequest(http.MethodGet, "/v1/jobs/by-name?name=named-job", nil) w := httptest.NewRecorder() handler.ServeHTTP(w, req) @@ -204,6 +733,76 @@ func TestServerGetJobByName(t *testing.T) { } } +func TestOperatorNameLookupQueryRoundTripsArbitraryNonEmptyNames(t *testing.T) { + store := newTestStore(t) + clock := newMockClock(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + scheduler := NewScheduler(store, &mockExecutor{}, clock, SchedulerConfig{}) + ts := httptest.NewServer(NewServer(store, scheduler, clock)) + t.Cleanup(ts.Close) + client := NewClient(ts.URL) + for i, name := range []string{"folder/nightly", "nightly report", "100% ready", "日本語-☃"} { + job, err := client.CreateJob(context.Background(), CreateJobRequest{Name: name, Schedule: "0 0 * * *", ExecType: ExecTypeShell, ExecConfig: `{"command":"echo ok"}`}) + if err != nil { + t.Fatalf("create %q: %v", name, err) + } + got, err := client.GetJobByName(context.Background(), name) + if err != nil { + t.Fatalf("GetJobByName(%q): %v", name, err) + } + if got.ID != job.ID || got.Name != name { + t.Fatalf("GetJobByName(%q) = %#v, want ID %q", name, got, job.ID) + } + if i == 0 { + if _, err := client.GetJob(context.Background(), name); err == nil { + t.Fatal("ID route accepted slash name") + } + } + } +} + +func TestOperatorNameLookupQueryRejectsEmptyAndNonGET(t *testing.T) { + handler, _ := newTestServer(t) + for _, tt := range []struct { + method, target string + status int + allow string + }{ + {http.MethodGet, "/v1/jobs/by-name", http.StatusBadRequest, ""}, + {http.MethodGet, "/v1/jobs/by-name?name=", http.StatusBadRequest, ""}, + {http.MethodPost, "/v1/jobs/by-name?name=job", http.StatusMethodNotAllowed, "GET"}, + } { + req := httptest.NewRequest(tt.method, tt.target, nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + if w.Code != tt.status { + t.Fatalf("%s %s status=%d body=%s, want %d", tt.method, tt.target, w.Code, w.Body.String(), tt.status) + } + if tt.allow != "" && w.Header().Get("Allow") != tt.allow { + t.Fatalf("%s %s Allow=%q, want %q", tt.method, tt.target, w.Header().Get("Allow"), tt.allow) + } + } + client := NewClient("http://127.0.0.1:1") + if _, err := client.GetJobByName(context.Background(), ""); err == nil || !strings.Contains(err.Error(), "name is required") { + t.Fatalf("empty client lookup error=%v, want name is required", err) + } +} + +func TestServerJobIDRouteNeverFallsBackToName(t *testing.T) { + handler, store := newTestServer(t) + store.GetJobFunc = func(context.Context, string) (Job, error) { return Job{}, sql.ErrNoRows } + nameLookups := 0 + store.GetJobByNameFunc = func(context.Context, string) (Job, error) { nameLookups++; return testJob("named-job"), nil } + req := httptest.NewRequest(http.MethodGet, "/v1/jobs/named-job", nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + if w.Code != http.StatusNotFound { + t.Fatalf("status = %d, body=%s", w.Code, w.Body.String()) + } + if nameLookups != 0 { + t.Fatalf("ID route performed %d name lookups", nameLookups) + } +} + func TestServerGetJobNotFound(t *testing.T) { handler, store := newTestServer(t) store.GetJobFunc = func(_ context.Context, id string) (Job, error) { @@ -416,6 +1015,75 @@ func TestServerUpdateJobInvalidJSON(t *testing.T) { } } +func TestServerUpdateJobRejectsStaleExpectedUpdatedAt(t *testing.T) { + handler, store := newTestServer(t) + job := testJob("stale-update") + job.UpdatedAt = time.Date(2026, 7, 31, 1, 0, 0, 0, time.UTC) + store.GetJobFunc = func(_ context.Context, id string) (Job, error) { + if id == job.ID { + return job, nil + } + return Job{}, sql.ErrNoRows + } + store.UpdateJobCASFunc = func(_ context.Context, _ Job, expected time.Time) error { + if !expected.Equal(job.UpdatedAt) { + return ErrJobConflict + } + return nil + } + + req := httptest.NewRequest(http.MethodPatch, "/v1/jobs/"+job.ID, strings.NewReader(`{"tags":"stale","expected_updated_at":"2026-07-31T00:00:00Z"}`)) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + if rec.Code != http.StatusConflict { + t.Fatalf("expected 409 conflict, got %d: %s", rec.Code, rec.Body.String()) + } +} + +func TestServerUpdateJobRejectsUnsafeExecutionConfig(t *testing.T) { + handler, store := newTestServer(t) + job := testJob("unsafe-update") + store.GetJobFunc = func(_ context.Context, id string) (Job, error) { + if id == job.ID { + return job, nil + } + return Job{}, sql.ErrNoRows + } + store.UpdateJobCASFunc = func(context.Context, Job, time.Time) error { return nil } + + tests := []struct { + name string + payload string + errMsg string + }{ + {"empty shell command", `{"execution_config":"{\"command\":\"\"}"}`, "non-empty command"}, + {"incomplete harness prompt", `{"execution_config":"{\"prompt\":\"\"}"}`, "non-empty command"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + current := job + if tt.name == "incomplete harness prompt" { + current.ExecType = ExecTypeHarness + current.ExecConfig = `{"prompt":"valid"}` + store.GetJobFunc = func(_ context.Context, id string) (Job, error) { + if id == current.ID { + return current, nil + } + return Job{}, sql.ErrNoRows + } + tt.errMsg = "non-empty prompt" + } + req := httptest.NewRequest(http.MethodPatch, "/v1/jobs/"+current.ID, strings.NewReader(tt.payload)) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusBadRequest || !strings.Contains(rec.Body.String(), tt.errMsg) { + t.Fatalf("response = %d %s, want actionable 400 containing %q", rec.Code, rec.Body.String(), tt.errMsg) + } + }) + } +} + func TestServerDeleteJob(t *testing.T) { handler, store := newTestServer(t) deleted := false @@ -750,7 +1418,7 @@ func TestServerCreateJobStoreError(t *testing.T) { return Job{}, fmt.Errorf("store failure") } - payload := `{"name":"err-job","schedule":"* * * * *","execution_type":"shell"}` + payload := `{"name":"err-job","schedule":"* * * * *","execution_type":"shell","execution_config":"{\"command\":\"echo hi\"}"}` req := httptest.NewRequest(http.MethodPost, "/v1/jobs", strings.NewReader(payload)) w := httptest.NewRecorder() handler.ServeHTTP(w, req) @@ -785,7 +1453,7 @@ func TestServerCreateJobDefaultTimeout(t *testing.T) { return job, nil } - payload := `{"name":"default-timeout","schedule":"* * * * *","execution_type":"shell"}` + payload := `{"name":"default-timeout","schedule":"* * * * *","execution_type":"shell","execution_config":"{\"command\":\"echo hi\"}"}` req := httptest.NewRequest(http.MethodPost, "/v1/jobs", strings.NewReader(payload)) w := httptest.NewRecorder() handler.ServeHTTP(w, req) diff --git a/internal/cron/store.go b/internal/cron/store.go index 06a0330a..4a5fee49 100644 --- a/internal/cron/store.go +++ b/internal/cron/store.go @@ -14,6 +14,7 @@ type Store interface { GetJobByName(ctx context.Context, name string) (Job, error) ListJobs(ctx context.Context) ([]Job, error) UpdateJob(ctx context.Context, job Job) error + UpdateJobCAS(ctx context.Context, job Job, expectedUpdatedAt time.Time) error // TouchJobRun updates only the run-tracking columns (last_run_at, // next_run_at, updated_at) for a job. Unlike UpdateJob, it never // touches schedule, execution config, status, timeout, or tags, so it @@ -21,6 +22,7 @@ type Store interface { // silent revert of concurrent user edits or resurrecting a paused job. TouchJobRun(ctx context.Context, jobID string, lastRun, nextRun, updatedAt time.Time) error DeleteJob(ctx context.Context, id string) error // soft delete + DeleteJobCAS(ctx context.Context, id string, expectedUpdatedAt time.Time) error CreateExecution(ctx context.Context, exec Execution) (Execution, error) UpdateExecution(ctx context.Context, exec Execution) error @@ -28,3 +30,12 @@ type Store interface { Close() error } + +// ScopedStore is implemented by stores that can enforce ownership in their +// lookup predicates. Server uses it when an authenticated conversational scope +// is supplied, while legacy operator calls remain supported through Store. +type ScopedStore interface { + GetJobInScope(ctx context.Context, id string, scope Scope) (Job, error) + GetJobByNameInScope(ctx context.Context, name string, scope Scope) (Job, error) + ListJobsInScope(ctx context.Context, scope Scope) ([]Job, error) +} diff --git a/internal/cron/store_sqlite.go b/internal/cron/store_sqlite.go index 970b3921..5eb6a6a6 100644 --- a/internal/cron/store_sqlite.go +++ b/internal/cron/store_sqlite.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "time" _ "modernc.org/sqlite" @@ -17,7 +18,7 @@ CREATE TABLE IF NOT EXISTS cron_jobs ( tenant_id TEXT NOT NULL DEFAULT '', conversation_id TEXT NOT NULL DEFAULT '', agent_id TEXT NOT NULL DEFAULT '', - name TEXT NOT NULL UNIQUE, + name TEXT NOT NULL, schedule TEXT NOT NULL, execution_type TEXT NOT NULL, execution_config TEXT NOT NULL DEFAULT '{}', @@ -92,9 +93,119 @@ func (s *SQLiteStore) Migrate(ctx context.Context) error { if err := s.ensureCronJobsScopeColumns(ctx); err != nil { return err } + if err := s.migrateScopedNameUniqueness(ctx); err != nil { + return err + } + if _, err := s.db.ExecContext(ctx, `CREATE UNIQUE INDEX IF NOT EXISTS idx_cron_jobs_active_scope_name ON cron_jobs(tenant_id, conversation_id, agent_id, name) WHERE status != 'deleted'`); err != nil { + return fmt.Errorf("index scoped cron names: %w", err) + } + return nil +} + +// migrateScopedNameUniqueness replaces the legacy global UNIQUE(name) +// constraint without dropping jobs or their execution history. SQLite cannot +// drop an inline uniqueness constraint, so rebuild both related tables in one +// transaction. The schema check makes repeat startups a no-op. +func (s *SQLiteStore) migrateScopedNameUniqueness(ctx context.Context) error { + legacyGlobalNameUnique, err := s.hasLegacyGlobalNameUnique(ctx) + if err != nil { + return err + } + if !legacyGlobalNameUnique { + return nil + } + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("begin scoped-name migration: %w", err) + } + defer func() { _ = tx.Rollback() }() + stmts := []string{ + `ALTER TABLE cron_executions RENAME TO cron_executions_legacy`, + `ALTER TABLE cron_jobs RENAME TO cron_jobs_legacy`, + `CREATE TABLE cron_jobs (job_id TEXT PRIMARY KEY, tenant_id TEXT NOT NULL DEFAULT '', conversation_id TEXT NOT NULL DEFAULT '', agent_id TEXT NOT NULL DEFAULT '', name TEXT NOT NULL, schedule TEXT NOT NULL, execution_type TEXT NOT NULL, execution_config TEXT NOT NULL DEFAULT '{}', status TEXT NOT NULL DEFAULT 'active', timeout_seconds INTEGER NOT NULL DEFAULT 30, tags TEXT NOT NULL DEFAULT '', next_run_at TIMESTAMP NOT NULL, last_run_at TIMESTAMP, created_at TIMESTAMP NOT NULL, updated_at TIMESTAMP NOT NULL)`, + `INSERT INTO cron_jobs SELECT job_id, tenant_id, conversation_id, agent_id, name, schedule, execution_type, execution_config, status, timeout_seconds, tags, next_run_at, last_run_at, created_at, updated_at FROM cron_jobs_legacy`, + `CREATE TABLE cron_executions (execution_id TEXT PRIMARY KEY, job_id TEXT NOT NULL, started_at TIMESTAMP NOT NULL, finished_at TIMESTAMP, status TEXT NOT NULL, run_id TEXT NOT NULL DEFAULT '', output_summary TEXT NOT NULL DEFAULT '', error_text TEXT NOT NULL DEFAULT '', duration_ms INTEGER NOT NULL DEFAULT 0, FOREIGN KEY (job_id) REFERENCES cron_jobs(job_id))`, + `INSERT INTO cron_executions SELECT execution_id, job_id, started_at, finished_at, status, run_id, output_summary, error_text, duration_ms FROM cron_executions_legacy`, + `DROP TABLE cron_executions_legacy`, `DROP TABLE cron_jobs_legacy`, + `CREATE INDEX idx_cron_executions_job_id ON cron_executions(job_id)`, `CREATE INDEX idx_cron_executions_started_at ON cron_executions(started_at)`, + `CREATE INDEX idx_cron_jobs_tenant_id ON cron_jobs(tenant_id)`, + `CREATE UNIQUE INDEX idx_cron_jobs_active_scope_name ON cron_jobs(tenant_id, conversation_id, agent_id, name) WHERE status != 'deleted'`, + } + for _, stmt := range stmts { + if _, err := tx.ExecContext(ctx, stmt); err != nil { + return fmt.Errorf("scoped-name migration: %w", err) + } + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("commit scoped-name migration: %w", err) + } return nil } +// hasLegacyGlobalNameUnique asks SQLite which indexes enforce cron_jobs +// uniqueness instead of parsing CREATE TABLE text. Inline, named, quoted, and +// collated UNIQUE(name) constraints all surface as a non-partial unique index +// with exactly one key column named "name". Composite scoped constraints and +// partial active-name indexes must not trigger a table rebuild. +func (s *SQLiteStore) hasLegacyGlobalNameUnique(ctx context.Context) (bool, error) { + rows, err := s.db.QueryContext(ctx, `SELECT name, "unique", origin, partial FROM pragma_index_list('cron_jobs')`) + if err != nil { + return false, fmt.Errorf("inspect cron_jobs indexes: %w", err) + } + type indexMetadata struct { + name string + unique bool + origin string + partial bool + } + var indexes []indexMetadata + for rows.Next() { + var index indexMetadata + if err := rows.Scan(&index.name, &index.unique, &index.origin, &index.partial); err != nil { + _ = rows.Close() + return false, fmt.Errorf("scan cron_jobs index: %w", err) + } + indexes = append(indexes, index) + } + if err := rows.Err(); err != nil { + _ = rows.Close() + return false, fmt.Errorf("inspect cron_jobs index rows: %w", err) + } + if err := rows.Close(); err != nil { + return false, fmt.Errorf("close cron_jobs index rows: %w", err) + } + + for _, index := range indexes { + if !index.unique || index.partial || index.origin == "pk" { + continue + } + columns, err := s.db.QueryContext(ctx, `SELECT name FROM pragma_index_xinfo(?) WHERE key = 1 ORDER BY seqno`, index.name) + if err != nil { + return false, fmt.Errorf("inspect cron_jobs index %q: %w", index.name, err) + } + var keyColumns []sql.NullString + for columns.Next() { + var name sql.NullString + if err := columns.Scan(&name); err != nil { + _ = columns.Close() + return false, fmt.Errorf("scan cron_jobs index %q: %w", index.name, err) + } + keyColumns = append(keyColumns, name) + } + if err := columns.Err(); err != nil { + _ = columns.Close() + return false, fmt.Errorf("inspect cron_jobs index %q rows: %w", index.name, err) + } + if err := columns.Close(); err != nil { + return false, fmt.Errorf("close cron_jobs index %q rows: %w", index.name, err) + } + if len(keyColumns) == 1 && keyColumns[0].Valid && strings.EqualFold(keyColumns[0].String, "name") { + return true, nil + } + } + return false, nil +} + func (s *SQLiteStore) ensureCronJobsScopeColumns(ctx context.Context) error { rows, err := s.db.QueryContext(ctx, `PRAGMA table_info(cron_jobs)`) if err != nil { @@ -179,14 +290,39 @@ WHERE job_id = ? AND status != ? // GetJobByName retrieves a job by name. func (s *SQLiteStore) GetJobByName(ctx context.Context, name string) (Job, error) { - return s.scanJob(s.db.QueryRowContext(ctx, ` + rows, err := s.db.QueryContext(ctx, ` SELECT job_id, tenant_id, name, schedule, execution_type, execution_config, conversation_id, agent_id, status, timeout_seconds, tags, next_run_at, last_run_at, created_at, updated_at FROM cron_jobs WHERE name = ? AND status != ? -`, name, StatusDeleted)) +ORDER BY created_at, job_id +LIMIT 2 +`, name, StatusDeleted) + if err != nil { + return Job{}, fmt.Errorf("get job by name: %w", err) + } + defer rows.Close() + if !rows.Next() { + return Job{}, sql.ErrNoRows + } + job, err := s.scanJobRow(rows) + if err != nil { + return Job{}, err + } + if rows.Next() { + return Job{}, ErrJobAmbiguous + } + return job, rows.Err() +} + +func (s *SQLiteStore) GetJobInScope(ctx context.Context, id string, scope Scope) (Job, error) { + return s.scanJob(s.db.QueryRowContext(ctx, `SELECT job_id, tenant_id, name, schedule, execution_type, execution_config, conversation_id, agent_id, status, timeout_seconds, tags, next_run_at, last_run_at, created_at, updated_at FROM cron_jobs WHERE job_id = ? AND tenant_id = ? AND conversation_id = ? AND agent_id = ? AND status != ?`, id, scope.TenantID, scope.ConversationID, scope.AgentID, StatusDeleted)) +} + +func (s *SQLiteStore) GetJobByNameInScope(ctx context.Context, name string, scope Scope) (Job, error) { + return s.scanJob(s.db.QueryRowContext(ctx, `SELECT job_id, tenant_id, name, schedule, execution_type, execution_config, conversation_id, agent_id, status, timeout_seconds, tags, next_run_at, last_run_at, created_at, updated_at FROM cron_jobs WHERE name = ? AND tenant_id = ? AND conversation_id = ? AND agent_id = ? AND status != ?`, name, scope.TenantID, scope.ConversationID, scope.AgentID, StatusDeleted)) } // ListJobs returns all non-deleted jobs. @@ -216,6 +352,23 @@ ORDER BY created_at DESC return jobs, rows.Err() } +func (s *SQLiteStore) ListJobsInScope(ctx context.Context, scope Scope) ([]Job, error) { + rows, err := s.db.QueryContext(ctx, `SELECT job_id, tenant_id, name, schedule, execution_type, execution_config, conversation_id, agent_id, status, timeout_seconds, tags, next_run_at, last_run_at, created_at, updated_at FROM cron_jobs WHERE tenant_id = ? AND conversation_id = ? AND agent_id = ? AND status != ? ORDER BY created_at DESC`, scope.TenantID, scope.ConversationID, scope.AgentID, StatusDeleted) + if err != nil { + return nil, fmt.Errorf("list scoped jobs: %w", err) + } + defer rows.Close() + var jobs []Job + for rows.Next() { + job, err := s.scanJobRow(rows) + if err != nil { + return nil, err + } + jobs = append(jobs, job) + } + return jobs, rows.Err() +} + // UpdateJob updates a job record. func (s *SQLiteStore) UpdateJob(ctx context.Context, job Job) error { _, err := s.db.ExecContext(ctx, ` @@ -247,6 +400,48 @@ WHERE job_id = ? return nil } +// UpdateJobCAS updates a job only when its persisted version still matches +// expectedUpdatedAt. The database row count is the authority for conflict +// detection; callers never perform a read/check/write sequence in memory. +func (s *SQLiteStore) UpdateJobCAS(ctx context.Context, job Job, expectedUpdatedAt time.Time) error { + res, err := s.db.ExecContext(ctx, ` +UPDATE cron_jobs +SET tenant_id = ?, name = ?, schedule = ?, execution_type = ?, execution_config = ?, + conversation_id = ?, agent_id = ?, + status = ?, timeout_seconds = ?, tags = ?, next_run_at = ?, + last_run_at = ?, updated_at = ? +WHERE job_id = ? AND status != ? AND updated_at = ? +`, + job.TenantID, + job.Name, + job.Schedule, + job.ExecType, + job.ExecConfig, + job.ConversationID, + job.AgentID, + job.Status, + job.TimeoutSec, + job.Tags, + nowString(job.NextRunAt), + nullableTimeString(job.LastRunAt), + nowString(job.UpdatedAt), + job.ID, + StatusDeleted, + nowString(expectedUpdatedAt), + ) + if err != nil { + return fmt.Errorf("compare-and-swap job: %w", err) + } + rows, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("compare-and-swap job rows affected: %w", err) + } + if rows == 0 { + return ErrJobConflict + } + return nil +} + // TouchJobRun updates only the run-tracking columns for a job // (last_run_at, next_run_at, updated_at), leaving schedule, execution // config, status, timeout, and tags untouched. This is used by the @@ -275,14 +470,13 @@ UPDATE cron_jobs SET last_run_at = ?, next_run_at = ?, updated_at = ? WHERE job_ } // DeleteJob performs a soft delete by setting status to deleted. -// It also renames the job to free the unique name constraint, -// allowing a new job with the same name to be created later. +// Scoped active-name uniqueness releases the name on delete without mutating +// the historical row's display name. func (s *SQLiteStore) DeleteJob(ctx context.Context, id string) error { now := time.Now().UTC() - suffix := fmt.Sprintf("_deleted_%d", now.UnixNano()) res, err := s.db.ExecContext(ctx, ` -UPDATE cron_jobs SET status = ?, name = name || ?, updated_at = ? WHERE job_id = ? -`, StatusDeleted, suffix, nowString(now), id) +UPDATE cron_jobs SET status = ?, updated_at = ? WHERE job_id = ? +`, StatusDeleted, nowString(now), id) if err != nil { return fmt.Errorf("delete job: %w", err) } @@ -296,6 +490,29 @@ UPDATE cron_jobs SET status = ?, name = name || ?, updated_at = ? WHERE job_id = return nil } +// DeleteJobCAS soft-deletes only the version the caller most recently read. +func (s *SQLiteStore) DeleteJobCAS(ctx context.Context, id string, expectedUpdatedAt time.Time) error { + now := time.Now().UTC() + if !now.After(expectedUpdatedAt) { + now = expectedUpdatedAt.Add(time.Nanosecond) + } + res, err := s.db.ExecContext(ctx, ` +UPDATE cron_jobs SET status = ?, updated_at = ? +WHERE job_id = ? AND status != ? AND updated_at = ? +`, StatusDeleted, nowString(now), id, StatusDeleted, nowString(expectedUpdatedAt)) + if err != nil { + return fmt.Errorf("compare-and-swap delete job: %w", err) + } + rows, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("compare-and-swap delete job rows affected: %w", err) + } + if rows == 0 { + return ErrJobConflict + } + return nil +} + // CreateExecution inserts a new execution record. func (s *SQLiteStore) CreateExecution(ctx context.Context, exec Execution) (Execution, error) { _, err := s.db.ExecContext(ctx, ` diff --git a/internal/cron/store_sqlite_test.go b/internal/cron/store_sqlite_test.go index 1e18c741..827f85ea 100644 --- a/internal/cron/store_sqlite_test.go +++ b/internal/cron/store_sqlite_test.go @@ -6,7 +6,6 @@ import ( "fmt" "os" "path/filepath" - "strings" "sync" "sync/atomic" "testing" @@ -112,6 +111,146 @@ INSERT INTO cron_jobs ( } } +// Legacy dumps used multiple identifier quoting styles for the same global +// UNIQUE(name) policy. Every spelling must rebuild without losing durable jobs +// or execution history. +func TestMigrate_RebuildsLegacyGlobalNameConstraintAndPreservesHistory(t *testing.T) { + variants := map[string]string{ + "bare": `CONSTRAINT cron_jobs_name_unique UNIQUE (name)`, + "quoted": `CONSTRAINT cron_jobs_name_unique UNIQUE ("name")`, + "bracketed": `CONSTRAINT cron_jobs_name_unique UNIQUE ([name])`, + "backtick": "CONSTRAINT cron_jobs_name_unique UNIQUE (`name`)", + "collated": `CONSTRAINT cron_jobs_name_unique UNIQUE (name COLLATE NOCASE)`, + } + for variant, constraint := range variants { + t.Run(variant, func(t *testing.T) { + store, err := NewSQLiteStore(filepath.Join(t.TempDir(), "legacy_unique.db")) + if err != nil { + t.Fatalf("NewSQLiteStore: %v", err) + } + t.Cleanup(func() { _ = store.Close() }) + ctx := context.Background() + jobsDDL := fmt.Sprintf(`CREATE TABLE cron_jobs ( + job_id TEXT PRIMARY KEY, tenant_id TEXT NOT NULL DEFAULT '', conversation_id TEXT NOT NULL DEFAULT '', agent_id TEXT NOT NULL DEFAULT '', + name TEXT NOT NULL, schedule TEXT NOT NULL, execution_type TEXT NOT NULL, execution_config TEXT NOT NULL DEFAULT '{}', + status TEXT NOT NULL DEFAULT 'active', timeout_seconds INTEGER NOT NULL DEFAULT 30, tags TEXT NOT NULL DEFAULT '', + next_run_at TIMESTAMP NOT NULL, last_run_at TIMESTAMP, created_at TIMESTAMP NOT NULL, updated_at TIMESTAMP NOT NULL, + %s + )`, constraint) + if _, err := store.db.ExecContext(ctx, jobsDDL); err != nil { + t.Fatalf("create legacy jobs: %v", err) + } + if _, err := store.db.ExecContext(ctx, `CREATE TABLE cron_executions (execution_id TEXT PRIMARY KEY, job_id TEXT NOT NULL, started_at TIMESTAMP NOT NULL, finished_at TIMESTAMP, status TEXT NOT NULL, run_id TEXT NOT NULL DEFAULT '', output_summary TEXT NOT NULL DEFAULT '', error_text TEXT NOT NULL DEFAULT '', duration_ms INTEGER NOT NULL DEFAULT 0, FOREIGN KEY (job_id) REFERENCES cron_jobs(job_id))`); err != nil { + t.Fatalf("create legacy executions: %v", err) + } + + created := time.Date(2026, 7, 30, 10, 11, 12, 123456789, time.UTC) + updated := created.Add(7 * time.Minute) + lastRun := created.Add(3 * time.Minute) + for i, name := range []string{"legacy-a", "legacy-b"} { + id := fmt.Sprintf("legacy-%d", i) + if _, err := store.db.ExecContext(ctx, `INSERT INTO cron_jobs (job_id, tenant_id, conversation_id, agent_id, name, schedule, execution_type, execution_config, status, timeout_seconds, tags, next_run_at, last_run_at, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, id, fmt.Sprintf("tenant-%d", i), "conversation", "agent", name, "*/5 * * * *", ExecTypeShell, `{"command":"echo ok"}`, StatusActive, 30, "legacy", nowString(updated.Add(time.Hour)), nowString(lastRun), nowString(created), nowString(updated)); err != nil { + t.Fatalf("insert legacy job %d: %v", i, err) + } + started, finished := created.Add(time.Duration(i)*time.Minute), created.Add(time.Duration(i)*time.Minute+15*time.Second) + if _, err := store.db.ExecContext(ctx, `INSERT INTO cron_executions (execution_id, job_id, started_at, finished_at, status, run_id, output_summary, error_text, duration_ms) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, fmt.Sprintf("exec-%d", i), id, nowString(started), nowString(finished), ExecStatusSuccess, fmt.Sprintf("run-%d", i), "preserved", "", 15000); err != nil { + t.Fatalf("insert legacy execution %d: %v", i, err) + } + } + + if err := store.Migrate(ctx); err != nil { + t.Fatalf("Migrate: %v", err) + } + if err := store.Migrate(ctx); err != nil { + t.Fatalf("second Migrate: %v", err) + } + var jobCount, executionCount int + if err := store.db.QueryRowContext(ctx, `SELECT COUNT(*) FROM cron_jobs`).Scan(&jobCount); err != nil { + t.Fatalf("count jobs: %v", err) + } + if err := store.db.QueryRowContext(ctx, `SELECT COUNT(*) FROM cron_executions`).Scan(&executionCount); err != nil { + t.Fatalf("count executions: %v", err) + } + if jobCount != 2 || executionCount != 2 { + t.Fatalf("post-migration counts jobs=%d executions=%d, want 2/2", jobCount, executionCount) + } + job, err := store.GetJob(ctx, "legacy-0") + if err != nil { + t.Fatalf("GetJob: %v", err) + } + if !job.CreatedAt.Equal(created) || !job.UpdatedAt.Equal(updated) || !job.LastRunAt.Equal(lastRun) { + t.Fatalf("job timestamps changed: %+v", job) + } + executions, err := store.ListExecutions(ctx, job.ID, 10, 0) + if err != nil || len(executions) != 1 || executions[0].ID != "exec-0" || executions[0].RunID != "run-0" || executions[0].DurationMs != 15000 { + t.Fatalf("history after migration = %#v, %v", executions, err) + } + + first, second := testJob("scope-shared"), testJob("scope-shared") + first.ID, second.ID = "scope-a", "scope-b" + first.TenantID, first.ConversationID, first.AgentID = "tenant-a", "conversation", "agent" + second.TenantID, second.ConversationID, second.AgentID = "tenant-b", "conversation", "agent" + if _, err := store.CreateJob(ctx, first); err != nil { + t.Fatalf("create first scope: %v", err) + } + if _, err := store.CreateJob(ctx, second); err != nil { + t.Fatalf("create second scope: %v", err) + } + duplicate := testJob("scope-shared") + duplicate.ID = "scope-a-duplicate" + duplicate.TenantID, duplicate.ConversationID, duplicate.AgentID = first.TenantID, first.ConversationID, first.AgentID + if _, err := store.CreateJob(ctx, duplicate); err == nil { + t.Fatal("same-scope duplicate name was accepted") + } + var integrity string + if err := store.db.QueryRowContext(ctx, `PRAGMA integrity_check`).Scan(&integrity); err != nil || integrity != "ok" { + t.Fatalf("integrity_check = %q, %v", integrity, err) + } + rows, err := store.db.QueryContext(ctx, `PRAGMA foreign_key_check`) + if err != nil { + t.Fatalf("foreign_key_check: %v", err) + } + defer rows.Close() + if rows.Next() { + t.Fatal("foreign_key_check reported a violation") + } + }) + } +} + +func TestLegacyGlobalNameUniqueMetadataIgnoresCompositeAndPartialIndexes(t *testing.T) { + store, err := NewSQLiteStore(filepath.Join(t.TempDir(), "index_metadata.db")) + if err != nil { + t.Fatalf("NewSQLiteStore: %v", err) + } + t.Cleanup(func() { _ = store.Close() }) + ctx := context.Background() + if _, err := store.db.ExecContext(ctx, `CREATE TABLE cron_jobs ( + job_id TEXT PRIMARY KEY, tenant_id TEXT NOT NULL, conversation_id TEXT NOT NULL, + agent_id TEXT NOT NULL, name TEXT NOT NULL, status TEXT NOT NULL, + UNIQUE (tenant_id, conversation_id, agent_id, name COLLATE NOCASE) + )`); err != nil { + t.Fatalf("create jobs: %v", err) + } + if _, err := store.db.ExecContext(ctx, `CREATE UNIQUE INDEX active_name_only ON cron_jobs(name COLLATE NOCASE) WHERE status != 'deleted'`); err != nil { + t.Fatalf("create partial index: %v", err) + } + legacy, err := store.hasLegacyGlobalNameUnique(ctx) + if err != nil { + t.Fatalf("inspect uniqueness: %v", err) + } + if legacy { + t.Fatal("composite and partial scoped indexes were misclassified as legacy global UNIQUE(name)") + } + if _, err := store.db.ExecContext(ctx, `CREATE UNIQUE INDEX legacy_global_name ON cron_jobs(name COLLATE NOCASE)`); err != nil { + t.Fatalf("create legacy global index: %v", err) + } + legacy, err = store.hasLegacyGlobalNameUnique(ctx) + if err != nil || !legacy { + t.Fatalf("global single-name index detected=%v, err=%v, want true", legacy, err) + } +} + func TestCreateJob_GetJob(t *testing.T) { store := newTestStore(t) ctx := context.Background() @@ -210,6 +349,22 @@ func TestGetJobByName(t *testing.T) { } } +func TestGetJobByName_AmbiguousAcrossScopes(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + for i, tenant := range []string{"tenant-a", "tenant-b"} { + job := testJob("shared-name") + job.ID = fmt.Sprintf("shared-%d", i) + job.TenantID, job.ConversationID, job.AgentID = tenant, "conversation", "agent" + if _, err := store.CreateJob(ctx, job); err != nil { + t.Fatalf("CreateJob(%s): %v", tenant, err) + } + } + if _, err := store.GetJobByName(ctx, "shared-name"); !IsJobAmbiguous(err) { + t.Fatalf("GetJobByName error = %v, want ErrJobAmbiguous", err) + } +} + func TestCreateJob_UniqueNameConstraint(t *testing.T) { store := newTestStore(t) ctx := context.Background() @@ -661,17 +816,14 @@ func TestSQLiteStore_DeleteAndRecreateSameName(t *testing.T) { t.Fatalf("expected name 'backup', got %q", created2.Name) } - // 4. Verify the old deleted job's name was changed. + // 4. Scoped active-name uniqueness preserves historical display names. var oldName string row := store.db.QueryRowContext(ctx, `SELECT name FROM cron_jobs WHERE job_id = ?`, created1.ID) if err := row.Scan(&oldName); err != nil { t.Fatalf("scan old job name: %v", err) } - if oldName == "backup" { - t.Fatal("expected deleted job's name to be changed, but it is still 'backup'") - } - if !strings.Contains(oldName, "backup_deleted_") { - t.Fatalf("expected deleted job name to contain 'backup_deleted_', got %q", oldName) + if oldName != "backup" { + t.Fatalf("expected deleted job name to be preserved, got %q", oldName) } } diff --git a/internal/cron/testutil_test.go b/internal/cron/testutil_test.go index b19b964a..b06b456d 100644 --- a/internal/cron/testutil_test.go +++ b/internal/cron/testutil_test.go @@ -16,8 +16,10 @@ type mockStore struct { GetJobByNameFunc func(ctx context.Context, name string) (Job, error) ListJobsFunc func(ctx context.Context) ([]Job, error) UpdateJobFunc func(ctx context.Context, job Job) error + UpdateJobCASFunc func(ctx context.Context, job Job, expectedUpdatedAt time.Time) error TouchJobRunFunc func(ctx context.Context, jobID string, lastRun, nextRun, updatedAt time.Time) error DeleteJobFunc func(ctx context.Context, id string) error + DeleteJobCASFunc func(ctx context.Context, id string, expectedUpdatedAt time.Time) error CreateExecutionFunc func(ctx context.Context, exec Execution) (Execution, error) UpdateExecutionFunc func(ctx context.Context, exec Execution) error ListExecutionsFunc func(ctx context.Context, jobID string, limit, offset int) ([]Execution, error) @@ -66,6 +68,13 @@ func (m *mockStore) UpdateJob(ctx context.Context, job Job) error { return nil } +func (m *mockStore) UpdateJobCAS(ctx context.Context, job Job, expectedUpdatedAt time.Time) error { + if m.UpdateJobCASFunc != nil { + return m.UpdateJobCASFunc(ctx, job, expectedUpdatedAt) + } + return m.UpdateJob(ctx, job) +} + func (m *mockStore) TouchJobRun(ctx context.Context, jobID string, lastRun, nextRun, updatedAt time.Time) error { if m.TouchJobRunFunc != nil { return m.TouchJobRunFunc(ctx, jobID, lastRun, nextRun, updatedAt) @@ -80,6 +89,13 @@ func (m *mockStore) DeleteJob(ctx context.Context, id string) error { return nil } +func (m *mockStore) DeleteJobCAS(ctx context.Context, id string, expectedUpdatedAt time.Time) error { + if m.DeleteJobCASFunc != nil { + return m.DeleteJobCASFunc(ctx, id, expectedUpdatedAt) + } + return m.DeleteJob(ctx, id) +} + func (m *mockStore) CreateExecution(ctx context.Context, exec Execution) (Execution, error) { if m.CreateExecutionFunc != nil { return m.CreateExecutionFunc(ctx, exec) diff --git a/internal/cron/types.go b/internal/cron/types.go index a7df0eeb..0875cf86 100644 --- a/internal/cron/types.go +++ b/internal/cron/types.go @@ -1,17 +1,44 @@ package cron import ( + "context" "database/sql" "errors" "time" ) +// Scope is the immutable ownership tuple for a conversational cron job. +type Scope struct{ TenantID, ConversationID, AgentID string } + +func (s Scope) Complete() bool { return s.TenantID != "" && s.ConversationID != "" && s.AgentID != "" } +func (s Scope) Matches(job Job) bool { + return s.TenantID == job.TenantID && s.ConversationID == job.ConversationID && s.AgentID == job.AgentID +} + +type scopeContextKey struct{} + +func WithScope(ctx context.Context, scope Scope) context.Context { + return context.WithValue(ctx, scopeContextKey{}, scope) +} +func ScopeFromContext(ctx context.Context) (Scope, bool) { + scope, ok := ctx.Value(scopeContextKey{}).(Scope) + return scope, ok && scope.Complete() +} + var ErrJobNotFound = errors.New("cron job not found") +var ErrJobConflict = errors.New("cron job update conflict") +var ErrJobAmbiguous = errors.New("cron job name is ambiguous") func IsJobNotFound(err error) bool { return errors.Is(err, ErrJobNotFound) || errors.Is(err, sql.ErrNoRows) } +func IsJobConflict(err error) bool { + return errors.Is(err, ErrJobConflict) +} + +func IsJobAmbiguous(err error) bool { return errors.Is(err, ErrJobAmbiguous) } + // Job status constants const ( StatusActive = "active" @@ -81,11 +108,19 @@ type CreateJobRequest struct { // UpdateJobRequest is the request payload for updating a job. type UpdateJobRequest struct { - Schedule *string `json:"schedule,omitempty"` - ExecConfig *string `json:"execution_config,omitempty"` - Status *string `json:"status,omitempty"` - TimeoutSec *int `json:"timeout_seconds,omitempty"` - Tags *string `json:"tags,omitempty"` + Schedule *string `json:"schedule,omitempty"` + ExecConfig *string `json:"execution_config,omitempty"` + Status *string `json:"status,omitempty"` + TimeoutSec *int `json:"timeout_seconds,omitempty"` + Tags *string `json:"tags,omitempty"` + ExpectedUpdatedAt *time.Time `json:"expected_updated_at,omitempty"` +} + +// DeleteJobRequest optionally carries an optimistic version. Operator callers +// may keep using an empty DELETE, while model-facing tools always supply the +// updated_at returned by cron_get. +type DeleteJobRequest struct { + ExpectedUpdatedAt *time.Time `json:"expected_updated_at,omitempty"` } // ListExecutionsRequest is the request payload for listing executions. diff --git a/internal/harness/default_registry_conditional_tools_test.go b/internal/harness/default_registry_conditional_tools_test.go index 288cf774..372e64e5 100644 --- a/internal/harness/default_registry_conditional_tools_test.go +++ b/internal/harness/default_registry_conditional_tools_test.go @@ -15,6 +15,7 @@ import ( "path/filepath" "strings" "testing" + "time" htools "go-agent-harness/internal/harness/tools" "go-agent-harness/internal/provider/catalog" @@ -35,6 +36,9 @@ func (stubCronClientForRegistryTest) UpdateJob(context.Context, string, htools.C return htools.CronJob{}, nil } func (stubCronClientForRegistryTest) DeleteJob(context.Context, string) error { return nil } +func (stubCronClientForRegistryTest) DeleteJobCAS(context.Context, string, time.Time) error { + return nil +} func (stubCronClientForRegistryTest) ListExecutions(context.Context, string, int, int) ([]htools.CronExecution, error) { return nil, nil } @@ -239,8 +243,8 @@ func TestCronToolsAreCoreNotDeferred(t *testing.T) { } for _, name := range []string{ - "cron_create", "cron_list", "cron_get", - "cron_delete", "cron_pause", "cron_resume", + "cron_create", "cron_list", "cron_get", "cron_update", + "cron_history", "cron_delete", "cron_pause", "cron_resume", } { if !visible[name] { t.Errorf("%q is not visible to a run without activation — the model cannot "+ @@ -249,7 +253,49 @@ func TestCronToolsAreCoreNotDeferred(t *testing.T) { } } -// TestNewDefaultRegistryWithOptions_CronToolRegistration pins that all six cron +// TestDefaultRegistryInitialCoreToolSchemasAreProviderCompatible exercises the +// actual tool list sent to a fresh model run. OpenAI-compatible providers +// require object-shaped function parameters and reject composition/constant +// keywords at the schema root; type-specific constraints such as a property +// enum remain valid below that root. +// +// Cron is included explicitly because all eight operations are core-visible: +// checking only cron_create would let a later CRUD schema change break the +// provider request before the model could create or manage a job. +func TestDefaultRegistryInitialCoreToolSchemasAreProviderCompatible(t *testing.T) { + t.Parallel() + + registry := NewDefaultRegistryWithOptions(t.TempDir(), DefaultRegistryOptions{ + ApprovalMode: ToolApprovalModeFullAuto, + CronClient: stubCronClientForRegistryTest{}, + }) + + checkedCron := make(map[string]bool) + for _, def := range registry.DefinitionsForRun("run-1", nil) { + if got := def.Parameters["type"]; got != "object" { + t.Errorf("initial core tool %q schema type = %#v, want object", def.Name, got) + } + for _, forbidden := range []string{"oneOf", "anyOf", "allOf", "enum", "const", "not"} { + if _, found := def.Parameters[forbidden]; found { + t.Errorf("initial core tool %q schema has forbidden top-level %q: %#v", def.Name, forbidden, def.Parameters) + } + } + if strings.HasPrefix(def.Name, "cron_") { + checkedCron[def.Name] = true + } + } + + for _, name := range []string{ + "cron_create", "cron_list", "cron_get", "cron_update", + "cron_history", "cron_delete", "cron_pause", "cron_resume", + } { + if !checkedCron[name] { + t.Errorf("%q was not checked in the initial core provider-schema set", name) + } + } +} + +// TestNewDefaultRegistryWithOptions_CronToolRegistration pins that all eight cron // tools are registered when a CronClient is configured, and that none of them // leak into a registry built without one. func TestNewDefaultRegistryWithOptions_CronToolRegistration(t *testing.T) { @@ -260,7 +306,10 @@ func TestNewDefaultRegistryWithOptions_CronToolRegistration(t *testing.T) { CronClient: stubCronClientForRegistryTest{}, }) present := registeredToolNames(withClient) - for _, name := range []string{"cron_create", "cron_list", "cron_get", "cron_delete", "cron_pause", "cron_resume"} { + for _, name := range []string{ + "cron_create", "cron_list", "cron_get", "cron_update", + "cron_history", "cron_delete", "cron_pause", "cron_resume", + } { if !present[name] { t.Errorf("cron tool %q not registered when a CronClient is configured", name) } diff --git a/internal/harness/tools/cron_test.go b/internal/harness/tools/cron_test.go index ebdf2c70..bf2a6dc2 100644 --- a/internal/harness/tools/cron_test.go +++ b/internal/harness/tools/cron_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "slices" "strings" "sync" "testing" @@ -59,6 +60,10 @@ func (m *mockCronClient) DeleteJob(ctx context.Context, id string) error { return nil } +func (m *mockCronClient) DeleteJobCAS(ctx context.Context, id string, _ time.Time) error { + return m.DeleteJob(ctx, id) +} + func (m *mockCronClient) ListExecutions(ctx context.Context, jobID string, limit, offset int) ([]tools.CronExecution, error) { if m.listExecsFn != nil { return m.listExecsFn(ctx, jobID, limit, offset) @@ -116,6 +121,23 @@ func TestCronCreate(t *testing.T) { if !tool.Definition.Mutating { t.Fatal("expected mutating=true") } + properties, ok := tool.Definition.Parameters["properties"].(map[string]any) + if !ok { + t.Fatal("expected cron_create properties schema") + } + timeoutSchema, ok := properties["timeout_seconds"].(map[string]any) + if !ok || timeoutSchema["minimum"] != 1 { + t.Fatalf("expected positive timeout schema, got %#v", properties["timeout_seconds"]) + } + if !strings.Contains(tool.Definition.Description, "non-empty") || !strings.Contains(tool.Definition.Description, "harness") { + t.Fatalf("description must explain shell and harness creation: %q", tool.Definition.Description) + } + if !strings.Contains(tool.Definition.Description, "set_delayed_callback") { + t.Fatalf("description must route one-shot delayed work to set_delayed_callback: %q", tool.Definition.Description) + } + if strings.Contains(strings.ToLower(tool.Definition.Description), "one-shot delayed execution, use bash") || strings.Contains(strings.ToLower(tool.Definition.Description), "sleep 120") { + t.Fatalf("description must not route one-shot delayed work through bash/sleep: %q", tool.Definition.Description) + } args := `{"name":"test-job","schedule":"*/5 * * * *","command":"echo hello"}` result, err := tool.Handler(context.Background(), json.RawMessage(args)) @@ -213,6 +235,53 @@ func TestCronCreate_UsesRunScopeAndIgnoresModelScopeOverrides(t *testing.T) { } } +func TestCronCreateHarnessJobUsesImmutableRunScope(t *testing.T) { + var gotReq tools.CronCreateJobRequest + client := &mockCronClient{ + createJobFn: func(_ context.Context, req tools.CronCreateJobRequest) (tools.CronJob, error) { + gotReq = req + return tools.CronJob{ID: "harness-job", ExecType: req.ExecType, ExecConfig: req.ExecConfig}, nil + }, + } + tool := deferred.CronCreateTool(client) + ctx := context.WithValue(context.Background(), tools.ContextKeyRunMetadata, tools.RunMetadata{ + TenantID: "tenant-a", ConversationID: "conversation-a", AgentID: "agent-a", + }) + + result, err := tool.Handler(ctx, json.RawMessage(`{"name":"conversational","schedule":"*/5 * * * *","execution_type":"harness","prompt":"Check deployment status","tenant_id":"spoof-tenant","conversation_id":"spoof-conversation","agent_id":"spoof-agent"}`)) + if err != nil { + t.Fatalf("cron_create harness: %v", err) + } + if gotReq.ExecType != "harness" { + t.Fatalf("execution type = %q, want harness", gotReq.ExecType) + } + if gotReq.ExecConfig != `{"prompt":"Check deployment status"}` { + t.Fatalf("execution config = %q, want typed prompt config", gotReq.ExecConfig) + } + if gotReq.TenantID != "tenant-a" || gotReq.ConversationID != "conversation-a" || gotReq.AgentID != "agent-a" { + t.Fatalf("model scope override was accepted: %+v", gotReq) + } + if !strings.Contains(result, "harness-job") { + t.Fatalf("result = %s, want created job", result) + } +} + +func TestCronCreateRejectsMixedShellAndHarnessInputs(t *testing.T) { + tool := deferred.CronCreateTool(&mockCronClient{}) + for name, args := range map[string]string{ + "harness without prompt": `{"name":"x","schedule":"* * * * *","execution_type":"harness"}`, + "harness with command": `{"name":"x","schedule":"* * * * *","execution_type":"harness","prompt":"p","command":"echo x"}`, + "shell with prompt": `{"name":"x","schedule":"* * * * *","execution_type":"shell","prompt":"p"}`, + "unknown type": `{"name":"x","schedule":"* * * * *","execution_type":"other","command":"echo x"}`, + } { + t.Run(name, func(t *testing.T) { + if _, err := tool.Handler(context.Background(), json.RawMessage(args)); err == nil { + t.Fatal("expected execution-type validation error") + } + }) + } +} + func TestCronList(t *testing.T) { t.Run("happy path", func(t *testing.T) { client := &mockCronClient{ @@ -315,6 +384,19 @@ func TestCronGet(t *testing.T) { if !strings.Contains(result, "recent_executions") { t.Errorf("result should contain recent_executions key") } + var parsed struct { + RecentExecutionsAvailable bool `json:"recent_executions_available"` + RecentExecutionsWarning string `json:"recent_executions_warning"` + } + if err := json.Unmarshal([]byte(result), &parsed); err != nil { + t.Fatalf("parse cron_get result: %v", err) + } + if !parsed.RecentExecutionsAvailable { + t.Fatal("successful history lookup must report recent_executions_available=true") + } + if parsed.RecentExecutionsWarning != "" { + t.Fatalf("successful history lookup warning = %q, want empty", parsed.RecentExecutionsWarning) + } }) t.Run("executions error degrades gracefully", func(t *testing.T) { @@ -334,9 +416,22 @@ func TestCronGet(t *testing.T) { if !strings.Contains(result, "test-job") { t.Errorf("result should still contain job") } - // Should have empty executions array - if !strings.Contains(result, "recent_executions") { - t.Errorf("result should contain recent_executions key") + var parsed struct { + RecentExecutions []tools.CronExecution `json:"recent_executions"` + RecentExecutionsAvailable bool `json:"recent_executions_available"` + RecentExecutionsWarning string `json:"recent_executions_warning"` + } + if err := json.Unmarshal([]byte(result), &parsed); err != nil { + t.Fatalf("parse cron_get result: %v", err) + } + if parsed.RecentExecutionsAvailable { + t.Fatal("failed history lookup must report recent_executions_available=false") + } + if !strings.Contains(parsed.RecentExecutionsWarning, "db error") { + t.Fatalf("history warning = %q, want the retrieval failure", parsed.RecentExecutionsWarning) + } + if parsed.RecentExecutions == nil || len(parsed.RecentExecutions) != 0 { + t.Fatalf("failed history lookup executions = %#v, want a non-nil empty array", parsed.RecentExecutions) } }) @@ -384,7 +479,7 @@ func TestCronDelete(t *testing.T) { t.Fatal("expected mutating=true") } - result, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1"}`)) + result, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -406,7 +501,7 @@ func TestCronDelete(t *testing.T) { }, } tool := deferred.CronDeleteTool(client) - _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1"}`)) + _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err == nil { t.Fatal("expected error") } @@ -416,6 +511,18 @@ func TestCronDelete(t *testing.T) { }) } +func TestCronDeleteRequiresExpectedVersion(t *testing.T) { + client := &mockCronClient{} + tool := deferred.CronDeleteTool(client) + required, ok := tool.Definition.Parameters["required"].([]string) + if !ok || !slices.Contains(required, "expected_updated_at") { + t.Fatalf("required schema = %#v, want expected_updated_at", tool.Definition.Parameters["required"]) + } + if _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1"}`)); err == nil || !strings.Contains(err.Error(), "expected_updated_at is required") { + t.Fatalf("missing version error = %v", err) + } +} + func TestCronPause(t *testing.T) { t.Run("happy path", func(t *testing.T) { var gotID string @@ -436,7 +543,7 @@ func TestCronPause(t *testing.T) { t.Fatalf("expected name cron_pause, got %s", tool.Definition.Name) } - result, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1"}`)) + result, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -458,7 +565,7 @@ func TestCronPause(t *testing.T) { }, } tool := deferred.CronPauseTool(client) - _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1"}`)) + _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err == nil { t.Fatal("expected error") } @@ -485,7 +592,7 @@ func TestCronResume(t *testing.T) { t.Fatalf("expected name cron_resume, got %s", tool.Definition.Name) } - result, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1"}`)) + result, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -507,7 +614,7 @@ func TestCronResume(t *testing.T) { }, } tool := deferred.CronResumeTool(client) - _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1"}`)) + _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err == nil { t.Fatal("expected error") } @@ -583,38 +690,29 @@ func TestCronCreateEmptyFields(t *testing.T) { }) t.Run("empty command", func(t *testing.T) { - var gotReq tools.CronCreateJobRequest - client := &mockCronClient{ - createJobFn: func(_ context.Context, req tools.CronCreateJobRequest) (tools.CronJob, error) { - gotReq = req - return testJob, nil - }, - } + client := &mockCronClient{} tool := deferred.CronCreateTool(client) _, err := tool.Handler(context.Background(), json.RawMessage(`{"name":"test","schedule":"* * * * *","command":""}`)) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if !strings.Contains(gotReq.ExecConfig, `"command":""`) { - t.Errorf("expected exec config to contain empty command, got %s", gotReq.ExecConfig) + if err == nil || !strings.Contains(err.Error(), "non-empty command") { + t.Fatalf("expected actionable empty-command error, got %v", err) } }) t.Run("negative timeout", func(t *testing.T) { - var gotReq tools.CronCreateJobRequest - client := &mockCronClient{ - createJobFn: func(_ context.Context, req tools.CronCreateJobRequest) (tools.CronJob, error) { - gotReq = req - return testJob, nil - }, - } + client := &mockCronClient{} tool := deferred.CronCreateTool(client) _, err := tool.Handler(context.Background(), json.RawMessage(`{"name":"test","schedule":"* * * * *","command":"echo","timeout_seconds":-1}`)) - if err != nil { - t.Fatalf("unexpected error: %v", err) + if err == nil || !strings.Contains(err.Error(), "timeout_seconds must be positive") { + t.Fatalf("expected actionable timeout error, got %v", err) } - if gotReq.TimeoutSec != -1 { - t.Errorf("expected negative timeout to pass through, got %d", gotReq.TimeoutSec) + }) + + t.Run("zero timeout", func(t *testing.T) { + client := &mockCronClient{} + tool := deferred.CronCreateTool(client) + _, err := tool.Handler(context.Background(), json.RawMessage(`{"name":"test","schedule":"* * * * *","command":"echo","timeout_seconds":0}`)) + if err == nil || !strings.Contains(err.Error(), "timeout_seconds must be positive") { + t.Fatalf("expected actionable timeout error, got %v", err) } }) } @@ -622,66 +720,50 @@ func TestCronCreateEmptyFields(t *testing.T) { // --- Regression tests: empty ID --- func TestCronGetEmptyID(t *testing.T) { - client := &mockCronClient{ - getJobFn: func(_ context.Context, id string) (tools.CronJob, error) { - return tools.CronJob{}, errors.New("not found: empty id") - }, - } + client := &mockCronClient{} tool := deferred.CronGetTool(client) _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":""}`)) if err == nil { t.Fatal("expected error for empty ID") } - if !strings.Contains(err.Error(), "empty id") { - t.Errorf("expected empty id error, got %v", err) + if !strings.Contains(err.Error(), "id is required") { + t.Errorf("expected required-ID error, got %v", err) } } func TestCronDeleteEmptyID(t *testing.T) { - client := &mockCronClient{ - deleteJobFn: func(_ context.Context, id string) error { - return errors.New("not found: empty id") - }, - } + client := &mockCronClient{} tool := deferred.CronDeleteTool(client) - _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":""}`)) + _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err == nil { t.Fatal("expected error for empty ID") } - if !strings.Contains(err.Error(), "empty id") { - t.Errorf("expected empty id error, got %v", err) + if !strings.Contains(err.Error(), "id is required") { + t.Errorf("expected required-ID error, got %v", err) } } func TestCronPauseEmptyID(t *testing.T) { - client := &mockCronClient{ - updateJobFn: func(_ context.Context, id string, _ tools.CronUpdateJobRequest) (tools.CronJob, error) { - return tools.CronJob{}, errors.New("not found: empty id") - }, - } + client := &mockCronClient{} tool := deferred.CronPauseTool(client) - _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":""}`)) + _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err == nil { t.Fatal("expected error for empty ID") } - if !strings.Contains(err.Error(), "empty id") { - t.Errorf("expected empty id error, got %v", err) + if !strings.Contains(err.Error(), "id is required") { + t.Errorf("expected required-ID error, got %v", err) } } func TestCronResumeEmptyID(t *testing.T) { - client := &mockCronClient{ - updateJobFn: func(_ context.Context, id string, _ tools.CronUpdateJobRequest) (tools.CronJob, error) { - return tools.CronJob{}, errors.New("not found: empty id") - }, - } + client := &mockCronClient{} tool := deferred.CronResumeTool(client) - _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":""}`)) + _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err == nil { t.Fatal("expected error for empty ID") } - if !strings.Contains(err.Error(), "empty id") { - t.Errorf("expected empty id error, got %v", err) + if !strings.Contains(err.Error(), "id is required") { + t.Errorf("expected required-ID error, got %v", err) } } @@ -716,15 +798,17 @@ func TestCronToolsConcurrentAccess(t *testing.T) { deferred.CronDeleteTool(client), deferred.CronPauseTool(client), deferred.CronResumeTool(client), + deferred.CronUpdateTool(client), } argsPerTool := []string{ `{"name":"test","schedule":"* * * * *","command":"echo hi"}`, `{}`, `{"id":"job-1"}`, - `{"id":"job-1"}`, - `{"id":"job-1"}`, - `{"id":"job-1"}`, + `{"id":"job-1","expected_updated_at":"2026-03-08T11:00:00Z"}`, + `{"id":"job-1","expected_updated_at":"2026-03-08T11:00:00Z"}`, + `{"id":"job-1","expected_updated_at":"2026-03-08T11:00:00Z"}`, + `{"id":"job-1","tags":"test","expected_updated_at":"2026-03-08T11:00:00Z"}`, } var wg sync.WaitGroup @@ -775,9 +859,10 @@ func TestCronToolsContextCancellation(t *testing.T) { {"cron_create", deferred.CronCreateTool(client), `{"name":"t","schedule":"*","command":"x"}`}, {"cron_list", deferred.CronListTool(client), `{}`}, {"cron_get", deferred.CronGetTool(client), `{"id":"1"}`}, - {"cron_delete", deferred.CronDeleteTool(client), `{"id":"1"}`}, - {"cron_pause", deferred.CronPauseTool(client), `{"id":"1"}`}, - {"cron_resume", deferred.CronResumeTool(client), `{"id":"1"}`}, + {"cron_delete", deferred.CronDeleteTool(client), `{"id":"1","expected_updated_at":"2026-03-08T11:00:00Z"}`}, + {"cron_pause", deferred.CronPauseTool(client), `{"id":"1","expected_updated_at":"2026-03-08T11:00:00Z"}`}, + {"cron_resume", deferred.CronResumeTool(client), `{"id":"1","expected_updated_at":"2026-03-08T11:00:00Z"}`}, + {"cron_update", deferred.CronUpdateTool(client), `{"id":"1","tags":"test","expected_updated_at":"2026-03-08T11:00:00Z"}`}, } for _, tc := range toolsAndArgs { @@ -839,10 +924,25 @@ func TestCronGetEmptyExecutionsIsArray(t *testing.T) { t.Fatal("missing recent_executions key in result") } - // The code sets execs = []tools.CronExecution{} on error, so it should be "[]" not "null" + // Keep the backward-compatible array shape, but never let the model confuse + // an unavailable history query with a successful query that found zero runs. if string(execsRaw) != "[]" { t.Errorf("expected recent_executions to be [], got %s", string(execsRaw)) } + var available bool + if err := json.Unmarshal(parsed["recent_executions_available"], &available); err != nil { + t.Fatalf("missing or invalid recent_executions_available: %v", err) + } + if available { + t.Fatal("failed history lookup must not be reported as available") + } + var warning string + if err := json.Unmarshal(parsed["recent_executions_warning"], &warning); err != nil { + t.Fatalf("missing or invalid recent_executions_warning: %v", err) + } + if !strings.Contains(warning, "db error") { + t.Fatalf("recent_executions_warning = %q, want db error", warning) + } } // --- Regression tests: constraint enforcement / idempotent operations --- @@ -854,7 +954,7 @@ func TestCronPauseAlreadyPaused(t *testing.T) { }, } tool := deferred.CronPauseTool(client) - _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1"}`)) + _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err == nil { t.Fatal("expected error") } @@ -870,7 +970,7 @@ func TestCronResumeAlreadyActive(t *testing.T) { }, } tool := deferred.CronResumeTool(client) - _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1"}`)) + _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err == nil { t.Fatal("expected error") } @@ -902,7 +1002,7 @@ func TestCronDeleteNonexistent(t *testing.T) { }, } tool := deferred.CronDeleteTool(client) - _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"nonexistent"}`)) + _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"nonexistent","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err == nil { t.Fatal("expected error") } diff --git a/internal/harness/tools/deferred/cron.go b/internal/harness/tools/deferred/cron.go index b51dad62..6e9ce541 100644 --- a/internal/harness/tools/deferred/cron.go +++ b/internal/harness/tools/deferred/cron.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "strings" + "time" tools "go-agent-harness/internal/harness/tools" "go-agent-harness/internal/harness/tools/descriptions" @@ -12,6 +13,24 @@ import ( func strPtr(s string) *string { return &s } +func requireCronJobID(id string) error { + if strings.TrimSpace(id) == "" { + return fmt.Errorf("id is required") + } + return nil +} + +func requiredExpectedUpdatedAt(value *string) (*time.Time, error) { + if value == nil || strings.TrimSpace(*value) == "" { + return nil, fmt.Errorf("expected_updated_at is required; call cron_get first") + } + parsed, err := time.Parse(time.RFC3339Nano, *value) + if err != nil { + return nil, fmt.Errorf("expected_updated_at must be an RFC3339 timestamp: %w", err) + } + return &parsed, nil +} + // CronCreateTool returns a deferred tool for creating cron jobs. func CronCreateTool(client tools.CronClient) tools.Tool { def := tools.Definition{ @@ -26,10 +45,12 @@ func CronCreateTool(client tools.CronClient) tools.Tool { "properties": map[string]any{ "name": map[string]any{"type": "string", "description": "Unique name for the cron job"}, "schedule": map[string]any{"type": "string", "description": "Standard 5-field cron expression: . All times are UTC. Must be a literal string — no shell substitutions or variables. Examples: \"*/5 * * * *\" = every 5 minutes, \"0 * * * *\" = every hour on the hour, \"30 2 * * *\" = daily at 02:30 UTC, \"0 9 * * 1-5\" = weekdays at 09:00 UTC, \"0 0 1 * *\" = first of every month at midnight UTC. To schedule relative to 'now', first run the bash tool to get the current UTC time, then compute the desired cron fields yourself."}, - "command": map[string]any{"type": "string", "description": "Shell command to execute on each trigger"}, - "timeout_seconds": map[string]any{"type": "integer", "description": "Max execution time in seconds (default 30). The job is killed if it exceeds this."}, + "execution_type": map[string]any{"type": "string", "enum": []string{"shell", "harness"}, "description": "shell for a legacy command or harness for a conversational prompt"}, + "command": map[string]any{"type": "string", "description": "Shell command to execute on each trigger; valid for execution_type shell"}, + "prompt": map[string]any{"type": "string", "description": "Prompt to send to the current conversation on each trigger; valid for execution_type harness"}, + "timeout_seconds": map[string]any{"type": "integer", "minimum": 1, "description": "Max execution time in seconds (default 30); must be positive. The job is killed if it exceeds this."}, }, - "required": []string{"name", "schedule", "command"}, + "required": []string{"name", "schedule"}, }, } @@ -37,17 +58,42 @@ func CronCreateTool(client tools.CronClient) tools.Tool { var args struct { Name string `json:"name"` Schedule string `json:"schedule"` + ExecutionType string `json:"execution_type"` Command string `json:"command"` - TimeoutSeconds int `json:"timeout_seconds"` + Prompt string `json:"prompt"` + TimeoutSeconds *int `json:"timeout_seconds"` } if err := json.Unmarshal(raw, &args); err != nil { return "", fmt.Errorf("parse cron_create args: %w", err) } - if args.TimeoutSeconds == 0 { - args.TimeoutSeconds = 30 - } - - execCfg, err := json.Marshal(map[string]string{"command": args.Command}) + timeoutSeconds := 30 + if args.TimeoutSeconds != nil { + if *args.TimeoutSeconds <= 0 { + return "", fmt.Errorf("timeout_seconds must be positive") + } + timeoutSeconds = *args.TimeoutSeconds + } + + executionType := strings.TrimSpace(args.ExecutionType) + if executionType == "" { + executionType = "shell" + } + var execCfg any + switch executionType { + case "shell": + if strings.TrimSpace(args.Command) == "" || strings.TrimSpace(args.Prompt) != "" { + return "", fmt.Errorf("shell cron_create requires a non-empty command and does not accept prompt") + } + execCfg = map[string]string{"command": args.Command} + case "harness": + if strings.TrimSpace(args.Prompt) == "" || strings.TrimSpace(args.Command) != "" { + return "", fmt.Errorf("harness cron_create requires prompt and does not accept command") + } + execCfg = map[string]string{"prompt": args.Prompt} + default: + return "", fmt.Errorf("execution_type must be shell or harness") + } + execConfig, err := json.Marshal(execCfg) if err != nil { return "", fmt.Errorf("marshal exec config: %w", err) } @@ -56,9 +102,9 @@ func CronCreateTool(client tools.CronClient) tools.Tool { job, err := client.CreateJob(ctx, tools.CronCreateJobRequest{ Name: args.Name, Schedule: args.Schedule, - ExecType: "shell", - ExecConfig: string(execCfg), - TimeoutSec: args.TimeoutSeconds, + ExecType: executionType, + ExecConfig: string(execConfig), + TimeoutSec: timeoutSeconds, TenantID: strings.TrimSpace(metadata.TenantID), ConversationID: strings.TrimSpace(metadata.ConversationID), AgentID: strings.TrimSpace(metadata.AgentID), @@ -110,7 +156,7 @@ func CronGetTool(client tools.CronClient) tools.Tool { Parameters: map[string]any{ "type": "object", "properties": map[string]any{ - "id": map[string]any{"type": "string", "description": "Job ID"}, + "id": map[string]any{"type": "string", "description": "Job ID only; names are not accepted"}, }, "required": []string{"id"}, }, @@ -123,6 +169,9 @@ func CronGetTool(client tools.CronClient) tools.Tool { if err := json.Unmarshal(raw, &args); err != nil { return "", fmt.Errorf("parse cron_get args: %w", err) } + if err := requireCronJobID(args.ID); err != nil { + return "", err + } job, err := client.GetJob(ctx, args.ID) if err != nil { @@ -130,13 +179,21 @@ func CronGetTool(client tools.CronClient) tools.Tool { } execs, execErr := client.ListExecutions(ctx, args.ID, 5, 0) + historyAvailable := execErr == nil if execErr != nil { execs = []tools.CronExecution{} } result := map[string]any{ - "job": job, - "recent_executions": execs, + "job": job, + "recent_executions": execs, + "recent_executions_available": historyAvailable, + } + if execErr != nil { + // Keep the job readable and preserve the established [] result shape, + // but never present an unavailable history query as proof that the job + // has not run. Models use this distinction to diagnose automations. + result["recent_executions_warning"] = fmt.Sprintf("recent execution history unavailable: %v", execErr) } return tools.MarshalToolResult(result) } @@ -156,21 +213,30 @@ func CronDeleteTool(client tools.CronClient) tools.Tool { Parameters: map[string]any{ "type": "object", "properties": map[string]any{ - "id": map[string]any{"type": "string", "description": "Job ID"}, + "id": map[string]any{"type": "string", "description": "Job ID only; names are not accepted"}, + "expected_updated_at": map[string]any{"type": "string", "format": "date-time", "description": "updated_at from cron_get; rejects stale delete requests"}, }, - "required": []string{"id"}, + "required": []string{"id", "expected_updated_at"}, }, } handler := func(ctx context.Context, raw json.RawMessage) (string, error) { var args struct { - ID string `json:"id"` + ID string `json:"id"` + ExpectedUpdatedAt *string `json:"expected_updated_at"` } if err := json.Unmarshal(raw, &args); err != nil { return "", fmt.Errorf("parse cron_delete args: %w", err) } + if err := requireCronJobID(args.ID); err != nil { + return "", err + } + expectedUpdatedAt, err := requiredExpectedUpdatedAt(args.ExpectedUpdatedAt) + if err != nil { + return "", err + } - if err := client.DeleteJob(ctx, args.ID); err != nil { + if err := client.DeleteJobCAS(ctx, args.ID, *expectedUpdatedAt); err != nil { return "", fmt.Errorf("cron_delete failed: %w", err) } @@ -195,22 +261,32 @@ func CronPauseTool(client tools.CronClient) tools.Tool { Parameters: map[string]any{ "type": "object", "properties": map[string]any{ - "id": map[string]any{"type": "string", "description": "Job ID"}, + "id": map[string]any{"type": "string", "description": "Job ID only; names are not accepted"}, + "expected_updated_at": map[string]any{"type": "string", "format": "date-time", "description": "updated_at from cron_get; rejects stale pause requests"}, }, - "required": []string{"id"}, + "required": []string{"id", "expected_updated_at"}, }, } handler := func(ctx context.Context, raw json.RawMessage) (string, error) { var args struct { - ID string `json:"id"` + ID string `json:"id"` + ExpectedUpdatedAt *string `json:"expected_updated_at"` } if err := json.Unmarshal(raw, &args); err != nil { return "", fmt.Errorf("parse cron_pause args: %w", err) } + if err := requireCronJobID(args.ID); err != nil { + return "", err + } + expectedUpdatedAt, err := requiredExpectedUpdatedAt(args.ExpectedUpdatedAt) + if err != nil { + return "", err + } job, err := client.UpdateJob(ctx, args.ID, tools.CronUpdateJobRequest{ - Status: strPtr("paused"), + Status: strPtr("paused"), + ExpectedUpdatedAt: expectedUpdatedAt, }) if err != nil { return "", fmt.Errorf("cron_pause failed: %w", err) @@ -233,22 +309,32 @@ func CronResumeTool(client tools.CronClient) tools.Tool { Parameters: map[string]any{ "type": "object", "properties": map[string]any{ - "id": map[string]any{"type": "string", "description": "Job ID"}, + "id": map[string]any{"type": "string", "description": "Job ID only; names are not accepted"}, + "expected_updated_at": map[string]any{"type": "string", "format": "date-time", "description": "updated_at from cron_get; rejects stale resume requests"}, }, - "required": []string{"id"}, + "required": []string{"id", "expected_updated_at"}, }, } handler := func(ctx context.Context, raw json.RawMessage) (string, error) { var args struct { - ID string `json:"id"` + ID string `json:"id"` + ExpectedUpdatedAt *string `json:"expected_updated_at"` } if err := json.Unmarshal(raw, &args); err != nil { return "", fmt.Errorf("parse cron_resume args: %w", err) } + if err := requireCronJobID(args.ID); err != nil { + return "", err + } + expectedUpdatedAt, err := requiredExpectedUpdatedAt(args.ExpectedUpdatedAt) + if err != nil { + return "", err + } job, err := client.UpdateJob(ctx, args.ID, tools.CronUpdateJobRequest{ - Status: strPtr("active"), + Status: strPtr("active"), + ExpectedUpdatedAt: expectedUpdatedAt, }) if err != nil { return "", fmt.Errorf("cron_resume failed: %w", err) @@ -259,145 +345,129 @@ func CronResumeTool(client tools.CronClient) tools.Tool { return tools.Tool{Definition: def, Handler: handler} } -// CronUpdateTool returns a tool for editing an existing cron job. -// -// UpdateJob has always supported changing a job's schedule, command, timeout -// and tags, but the only tools built on it were pause and resume, which set -// status alone. So an agent asked to "run that hourly instead" had to delete -// the job and create a new one — losing its ID and, with it, the execution -// history that is the only record of whether the thing ever worked. -func CronUpdateTool(client tools.CronClient) tools.Tool { - def := tools.Definition{ - Name: "cron_update", - Description: descriptions.Load("cron_update"), - Action: tools.ActionExecute, - Mutating: true, - Tier: tools.TierDeferred, - Tags: []string{"cron", "schedule", "automation"}, - Parameters: map[string]any{ - "type": "object", - "properties": map[string]any{ - "id": map[string]any{"type": "string", "description": "Job ID"}, - "schedule": map[string]any{ - "type": "string", - "description": "New cron expression, e.g. '0 * * * *'", - }, - "execution_config": map[string]any{ - "type": "string", - "description": "New execution config as JSON, e.g. " + - `{"command":"echo hi"} for shell, ` + - `{"prompt":"..."} for harness`, - }, - "timeout_seconds": map[string]any{ - "type": "integer", - "description": "New timeout in seconds", - }, - "tags": map[string]any{"type": "string", "description": "New tags"}, - }, - "required": []string{"id"}, - }, - } - +func CronHistoryTool(client tools.CronClient) tools.Tool { + def := tools.Definition{Name: "cron_history", Description: descriptions.Load("cron_history"), Action: tools.ActionRead, ParallelSafe: true, Tier: tools.TierDeferred, Tags: []string{"cron", "schedule", "automation"}, Parameters: map[string]any{"type": "object", "properties": map[string]any{"id": map[string]any{"type": "string", "description": "Job ID only; names are not accepted"}, "limit": map[string]any{"type": "integer"}, "offset": map[string]any{"type": "integer"}}, "required": []string{"id"}}} handler := func(ctx context.Context, raw json.RawMessage) (string, error) { var args struct { - ID string `json:"id"` - Schedule *string `json:"schedule"` - ExecConfig *string `json:"execution_config"` - TimeoutSec *int `json:"timeout_seconds"` - Tags *string `json:"tags"` + ID string `json:"id"` + Limit int `json:"limit"` + Offset int `json:"offset"` } if err := json.Unmarshal(raw, &args); err != nil { - return "", fmt.Errorf("parse cron_update args: %w", err) + return "", fmt.Errorf("parse cron_history args: %w", err) + } + if err := requireCronJobID(args.ID); err != nil { + return "", err + } + if args.Limit <= 0 { + args.Limit = 20 } - if args.ID == "" { - return "", fmt.Errorf("id is required") + if args.Limit > 100 { + args.Limit = 100 } - // Reject a no-op rather than reporting a successful update that - // changed nothing — the caller almost certainly meant a field name - // this tool does not accept. - if args.Schedule == nil && args.ExecConfig == nil && - args.TimeoutSec == nil && args.Tags == nil { - return "", fmt.Errorf( - "cron_update needs at least one of schedule, execution_config, " + - "timeout_seconds or tags; use cron_pause/cron_resume to change status") + if args.Offset < 0 { + args.Offset = 0 } - - job, err := client.UpdateJob(ctx, args.ID, tools.CronUpdateJobRequest{ - Schedule: args.Schedule, - ExecConfig: args.ExecConfig, - TimeoutSec: args.TimeoutSec, - Tags: args.Tags, - }) + execs, err := client.ListExecutions(ctx, args.ID, args.Limit, args.Offset) if err != nil { - return "", fmt.Errorf("cron_update failed: %w", err) + return "", fmt.Errorf("cron_history failed: %w", err) } - return tools.MarshalToolResult(job) + return tools.MarshalToolResult(map[string]any{"job_id": args.ID, "executions": execs, "count": len(execs)}) } - return tools.Tool{Definition: def, Handler: handler} } -// CronHistoryTool returns a tool for reading a job's execution history. -// -// cron_get already returns the five most recent executions, which answers -// "did it run?" but not "has it been failing since Tuesday?". This exposes the -// paging the client has always supported. -func CronHistoryTool(client tools.CronClient) tools.Tool { +// CronUpdateTool returns a deferred tool for editing an existing cron job in place. +// Omitted fields are preserved by the pointer-based update request. Status is +// deliberately kept behind the explicit pause/resume tools. +func CronUpdateTool(client tools.CronClient) tools.Tool { def := tools.Definition{ - Name: "cron_history", - Description: descriptions.Load("cron_history"), - Action: tools.ActionRead, - ParallelSafe: true, - Tier: tools.TierDeferred, - Tags: []string{"cron", "schedule", "automation"}, + Name: "cron_update", + Description: descriptions.Load("cron_update"), + Action: tools.ActionExecute, + Mutating: true, + Tier: tools.TierDeferred, + Tags: []string{"cron", "schedule", "automation"}, Parameters: map[string]any{ "type": "object", "properties": map[string]any{ - "id": map[string]any{"type": "string", "description": "Job ID"}, - "limit": map[string]any{ - "type": "integer", - "description": "Executions to return (default 20, max 100)", - }, - "offset": map[string]any{ - "type": "integer", - "description": "Executions to skip, for paging back in time", - }, + "id": map[string]any{"type": "string", "description": "Job ID only; names are not accepted"}, + "schedule": map[string]any{"type": "string", "description": "New 5-field UTC cron expression"}, + "command": map[string]any{"type": "string", "description": "New shell command; encoded as execution_config"}, + "prompt": map[string]any{"type": "string", "description": "New harness prompt; encoded as execution_config"}, + "execution_config": map[string]any{"type": "string", "description": "New execution config JSON"}, + "timeout_seconds": map[string]any{"type": "integer", "minimum": 1, "description": "New positive timeout in seconds"}, + "tags": map[string]any{"type": "string", "description": "Replacement comma-separated tags"}, + "expected_updated_at": map[string]any{"type": "string", "format": "date-time", "description": "updated_at from cron_get; rejects stale writes"}, }, - "required": []string{"id"}, + "required": []string{"id", "expected_updated_at"}, }, } handler := func(ctx context.Context, raw json.RawMessage) (string, error) { var args struct { - ID string `json:"id"` - Limit int `json:"limit"` - Offset int `json:"offset"` + ID string `json:"id"` + Schedule *string `json:"schedule"` + Command *string `json:"command"` + Prompt *string `json:"prompt"` + ExecConfig *string `json:"execution_config"` + TimeoutSec *int `json:"timeout_seconds"` + Tags *string `json:"tags"` + ExpectedUpdatedAt *string `json:"expected_updated_at"` } if err := json.Unmarshal(raw, &args); err != nil { - return "", fmt.Errorf("parse cron_history args: %w", err) + return "", fmt.Errorf("parse cron_update args: %w", err) } - if args.ID == "" { - return "", fmt.Errorf("id is required") + if err := requireCronJobID(args.ID); err != nil { + return "", err } - if args.Limit <= 0 { - args.Limit = 20 + expectedUpdatedAt, err := requiredExpectedUpdatedAt(args.ExpectedUpdatedAt) + if err != nil { + return "", err } - if args.Limit > 100 { - args.Limit = 100 + executionInputs := 0 + for _, present := range []bool{args.Command != nil, args.Prompt != nil, args.ExecConfig != nil} { + if present { + executionInputs++ + } } - if args.Offset < 0 { - args.Offset = 0 + if executionInputs > 1 { + return "", fmt.Errorf("provide only one of command, prompt, or execution_config") + } + if args.Command != nil { + encoded, err := json.Marshal(map[string]string{"command": *args.Command}) + if err != nil { + return "", fmt.Errorf("encode command: %w", err) + } + config := string(encoded) + args.ExecConfig = &config + } + if args.Prompt != nil { + encoded, err := json.Marshal(map[string]string{"prompt": *args.Prompt}) + if err != nil { + return "", fmt.Errorf("encode prompt: %w", err) + } + config := string(encoded) + args.ExecConfig = &config + } + if args.TimeoutSec != nil && *args.TimeoutSec <= 0 { + return "", fmt.Errorf("timeout_seconds must be positive") + } + if args.Schedule == nil && args.ExecConfig == nil && args.TimeoutSec == nil && args.Tags == nil { + return "", fmt.Errorf("cron_update needs at least one of schedule, command, prompt, execution_config, timeout_seconds or tags; use cron_pause/cron_resume to change status") } - execs, err := client.ListExecutions(ctx, args.ID, args.Limit, args.Offset) + job, err := client.UpdateJob(ctx, args.ID, tools.CronUpdateJobRequest{ + Schedule: args.Schedule, + ExecConfig: args.ExecConfig, + TimeoutSec: args.TimeoutSec, + Tags: args.Tags, + ExpectedUpdatedAt: expectedUpdatedAt, + }) if err != nil { - return "", fmt.Errorf("cron_history failed: %w", err) + return "", fmt.Errorf("cron_update failed: %w", err) } - return tools.MarshalToolResult(map[string]any{ - "job_id": args.ID, - "executions": execs, - "count": len(execs), - }) + return tools.MarshalToolResult(job) } return tools.Tool{Definition: def, Handler: handler} diff --git a/internal/harness/tools/deferred/cron_extra_test.go b/internal/harness/tools/deferred/cron_extra_test.go index c3fde4f4..ea9cc18d 100644 --- a/internal/harness/tools/deferred/cron_extra_test.go +++ b/internal/harness/tools/deferred/cron_extra_test.go @@ -3,12 +3,39 @@ package deferred import ( "context" "encoding/json" + "fmt" "strings" "testing" + "time" tools "go-agent-harness/internal/harness/tools" ) +type blankIDRecordingClient struct { + tools.CronClient + calls int +} + +func (c *blankIDRecordingClient) GetJob(context.Context, string) (tools.CronJob, error) { + c.calls++ + return tools.CronJob{}, nil +} + +func (c *blankIDRecordingClient) UpdateJob(context.Context, string, tools.CronUpdateJobRequest) (tools.CronJob, error) { + c.calls++ + return tools.CronJob{}, nil +} + +func (c *blankIDRecordingClient) DeleteJobCAS(context.Context, string, time.Time) error { + c.calls++ + return nil +} + +func (c *blankIDRecordingClient) ListExecutions(context.Context, string, int, int) ([]tools.CronExecution, error) { + c.calls++ + return nil, nil +} + type recordingCronClient struct { tools.CronClient lastUpdateID string @@ -41,7 +68,7 @@ func TestCronUpdateChangesOnlyTheFieldsGiven(t *testing.T) { tool := CronUpdateTool(client) _, err := tool.Handler(context.Background(), - json.RawMessage(`{"id":"job-1","schedule":"0 * * * *"}`)) + json.RawMessage(`{"id":"job-1","schedule":"0 * * * *","expected_updated_at":"2026-08-01T00:00:00Z"}`)) if err != nil { t.Fatalf("cron_update: %v", err) } @@ -61,7 +88,7 @@ func TestCronUpdateChangesOnlyTheFieldsGiven(t *testing.T) { // name this tool does not accept; reporting success would hide that. func TestCronUpdateRejectsANoOp(t *testing.T) { tool := CronUpdateTool(&recordingCronClient{}) - _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1"}`)) + _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","expected_updated_at":"2026-08-01T00:00:00Z"}`)) if err == nil { t.Fatal("a no-op update was accepted") } @@ -94,3 +121,55 @@ func TestCronHistoryClampsPaging(t *testing.T) { t.Errorf("negative offset became %d, want 0", client.lastExecOff) } } + +func TestCronCRUDSchemasRequireJobIDAndDoNotAdvertiseNames(t *testing.T) { + client := &recordingCronClient{} + for _, tool := range []tools.Tool{CronGetTool(client), CronUpdateTool(client), CronPauseTool(client), CronResumeTool(client), CronDeleteTool(client), CronHistoryTool(client)} { + props := tool.Definition.Parameters["properties"].(map[string]any) + if _, ok := props["name"]; ok { + t.Fatalf("%s advertises forbidden name lookup", tool.Definition.Name) + } + id := props["id"].(map[string]any) + description, _ := id["description"].(string) + if !strings.Contains(strings.ToLower(description), "id only") { + t.Fatalf("%s id description = %q, want explicit ID-only contract", tool.Definition.Name, description) + } + } +} + +func TestCronExistingJobToolsRejectBlankIDsBeforeClientCalls(t *testing.T) { + const version = "2026-08-01T00:00:00Z" + for _, id := range []string{"", " \t "} { + for _, tc := range []struct { + name string + tool func(tools.CronClient) tools.Tool + args func(string) string + }{ + {name: "get", tool: CronGetTool, args: func(id string) string { return fmt.Sprintf(`{"id":%q}`, id) }}, + {name: "delete", tool: CronDeleteTool, args: func(id string) string { + return fmt.Sprintf(`{"id":%q,"expected_updated_at":%q}`, id, version) + }}, + {name: "pause", tool: CronPauseTool, args: func(id string) string { + return fmt.Sprintf(`{"id":%q,"expected_updated_at":%q}`, id, version) + }}, + {name: "resume", tool: CronResumeTool, args: func(id string) string { + return fmt.Sprintf(`{"id":%q,"expected_updated_at":%q}`, id, version) + }}, + {name: "update", tool: CronUpdateTool, args: func(id string) string { + return fmt.Sprintf(`{"id":%q,"tags":"changed","expected_updated_at":%q}`, id, version) + }}, + {name: "history", tool: CronHistoryTool, args: func(id string) string { return fmt.Sprintf(`{"id":%q}`, id) }}, + } { + t.Run(fmt.Sprintf("%s/%q", tc.name, id), func(t *testing.T) { + client := &blankIDRecordingClient{} + _, err := tc.tool(client).Handler(context.Background(), json.RawMessage(tc.args(id))) + if err == nil || !strings.Contains(err.Error(), "id is required") { + t.Fatalf("error = %v, want id is required", err) + } + if client.calls != 0 { + t.Fatalf("client calls = %d, want 0", client.calls) + } + }) + } + } +} diff --git a/internal/harness/tools/deferred/cron_scope.go b/internal/harness/tools/deferred/cron_scope.go new file mode 100644 index 00000000..99599496 --- /dev/null +++ b/internal/harness/tools/deferred/cron_scope.go @@ -0,0 +1,144 @@ +package deferred + +import ( + "context" + "fmt" + "strings" + "time" + + "go-agent-harness/internal/cron" + tools "go-agent-harness/internal/harness/tools" +) + +// NewScopedCronClient confines every model-facing cron operation to the +// immutable tenant, conversation, and agent scope carried by RunMetadata. +// The wrapper belongs at the tool boundary so embedded and remote cron clients +// enforce the same ownership contract without changing their operator APIs. +func NewScopedCronClient(client tools.CronClient) tools.CronClient { + if _, alreadyScoped := client.(*scopedCronClient); alreadyScoped { + return client + } + return &scopedCronClient{client: client} +} + +func scopedCronContext(ctx context.Context, metadata tools.RunMetadata) context.Context { + return cron.WithScope(ctx, cron.Scope{TenantID: metadata.TenantID, ConversationID: metadata.ConversationID, AgentID: metadata.AgentID}) +} + +type scopedCronClient struct { + client tools.CronClient +} + +func cronScopeFromContext(ctx context.Context) (tools.RunMetadata, error) { + metadata, ok := tools.RunMetadataFromContext(ctx) + if !ok || + strings.TrimSpace(metadata.TenantID) == "" || + strings.TrimSpace(metadata.ConversationID) == "" || + strings.TrimSpace(metadata.AgentID) == "" { + return tools.RunMetadata{}, fmt.Errorf("cron scope is required") + } + metadata.TenantID = strings.TrimSpace(metadata.TenantID) + metadata.ConversationID = strings.TrimSpace(metadata.ConversationID) + metadata.AgentID = strings.TrimSpace(metadata.AgentID) + return metadata, nil +} + +func cronJobInScope(job tools.CronJob, metadata tools.RunMetadata) bool { + return job.TenantID == metadata.TenantID && + job.ConversationID == metadata.ConversationID && + job.AgentID == metadata.AgentID +} + +func (c *scopedCronClient) CreateJob(ctx context.Context, req tools.CronCreateJobRequest) (tools.CronJob, error) { + metadata, err := cronScopeFromContext(ctx) + if err != nil { + return tools.CronJob{}, err + } + if strings.TrimSpace(req.TenantID) != metadata.TenantID || + strings.TrimSpace(req.ConversationID) != metadata.ConversationID || + strings.TrimSpace(req.AgentID) != metadata.AgentID { + return tools.CronJob{}, fmt.Errorf("cron create scope does not match the active run") + } + return c.client.CreateJob(scopedCronContext(ctx, metadata), req) +} + +func (c *scopedCronClient) ListJobs(ctx context.Context) ([]tools.CronJob, error) { + metadata, err := cronScopeFromContext(ctx) + if err != nil { + return nil, err + } + jobs, err := c.client.ListJobs(scopedCronContext(ctx, metadata)) + if err != nil { + return nil, err + } + filtered := make([]tools.CronJob, 0, len(jobs)) + for _, job := range jobs { + if cronJobInScope(job, metadata) { + filtered = append(filtered, job) + } + } + return filtered, nil +} + +func (c *scopedCronClient) GetJob(ctx context.Context, id string) (tools.CronJob, error) { + metadata, err := cronScopeFromContext(ctx) + if err != nil { + return tools.CronJob{}, err + } + job, err := c.client.GetJob(scopedCronContext(ctx, metadata), id) + if err != nil { + return tools.CronJob{}, err + } + if !cronJobInScope(job, metadata) { + return tools.CronJob{}, tools.ErrCronJobNotFound + } + return job, nil +} + +func (c *scopedCronClient) UpdateJob(ctx context.Context, id string, req tools.CronUpdateJobRequest) (tools.CronJob, error) { + metadata, err := cronScopeFromContext(ctx) + if err != nil { + return tools.CronJob{}, err + } + if _, err := c.GetJob(ctx, id); err != nil { + return tools.CronJob{}, err + } + return c.client.UpdateJob(scopedCronContext(ctx, metadata), id, req) +} + +func (c *scopedCronClient) DeleteJob(ctx context.Context, id string) error { + metadata, err := cronScopeFromContext(ctx) + if err != nil { + return err + } + if _, err := c.GetJob(ctx, id); err != nil { + return err + } + return c.client.DeleteJob(scopedCronContext(ctx, metadata), id) +} + +func (c *scopedCronClient) DeleteJobCAS(ctx context.Context, id string, expectedUpdatedAt time.Time) error { + metadata, err := cronScopeFromContext(ctx) + if err != nil { + return err + } + if _, err := c.GetJob(ctx, id); err != nil { + return err + } + return c.client.DeleteJobCAS(scopedCronContext(ctx, metadata), id, expectedUpdatedAt) +} + +func (c *scopedCronClient) ListExecutions(ctx context.Context, jobID string, limit, offset int) ([]tools.CronExecution, error) { + metadata, err := cronScopeFromContext(ctx) + if err != nil { + return nil, err + } + if _, err := c.GetJob(ctx, jobID); err != nil { + return nil, err + } + return c.client.ListExecutions(scopedCronContext(ctx, metadata), jobID, limit, offset) +} + +func (c *scopedCronClient) Health(ctx context.Context) error { + return c.client.Health(ctx) +} diff --git a/internal/harness/tools/deferred/cron_update_test.go b/internal/harness/tools/deferred/cron_update_test.go new file mode 100644 index 00000000..eac9371e --- /dev/null +++ b/internal/harness/tools/deferred/cron_update_test.go @@ -0,0 +1,243 @@ +package deferred + +import ( + "context" + "encoding/json" + "errors" + "slices" + "strings" + "testing" + "time" + + "go-agent-harness/internal/cron" + tools "go-agent-harness/internal/harness/tools" +) + +type recordingCronUpdateClient struct { + tools.CronClient + lastID string + lastReq tools.CronUpdateJobRequest + err error +} + +func (c *recordingCronUpdateClient) UpdateJob(_ context.Context, id string, req tools.CronUpdateJobRequest) (tools.CronJob, error) { + c.lastID = id + c.lastReq = req + if c.err != nil { + return tools.CronJob{}, c.err + } + return tools.CronJob{ID: id, Schedule: "0 * * * *", UpdatedAt: time.Date(2026, 7, 31, 0, 0, 0, 0, time.UTC)}, nil +} + +func TestCronUpdateChangesOnlyTheFieldsGivenWithExpectedVersion(t *testing.T) { + client := &recordingCronUpdateClient{} + tool := CronUpdateTool(client) + + result, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","schedule":"0 * * * *","expected_updated_at":"2026-07-31T00:00:00Z"}`)) + if err != nil { + t.Fatalf("cron_update: %v", err) + } + if client.lastID != "job-1" { + t.Fatalf("updated %q, want job-1", client.lastID) + } + if client.lastReq.Schedule == nil || *client.lastReq.Schedule != "0 * * * *" { + t.Fatal("schedule was not forwarded") + } + if client.lastReq.ExecConfig != nil || client.lastReq.TimeoutSec != nil || client.lastReq.Tags != nil { + t.Fatal("omitted fields were forwarded and could overwrite existing values") + } + if !strings.Contains(result, "job-1") { + t.Fatalf("result did not contain updated job: %s", result) + } +} + +func TestCronUpdateAcceptsCommandAndExpectedTimestamp(t *testing.T) { + client := &recordingCronUpdateClient{} + tool := CronUpdateTool(client) + + _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","command":"echo ready","expected_updated_at":"2026-07-30T23:00:00Z"}`)) + if err != nil { + t.Fatalf("cron_update: %v", err) + } + if client.lastReq.ExecConfig == nil || *client.lastReq.ExecConfig != `{"command":"echo ready"}` { + t.Fatalf("command was not encoded as execution config: %#v", client.lastReq.ExecConfig) + } + want := time.Date(2026, 7, 30, 23, 0, 0, 0, time.UTC) + if client.lastReq.ExpectedUpdatedAt == nil || !client.lastReq.ExpectedUpdatedAt.Equal(want) { + t.Fatalf("expected timestamp = %v, got %#v", want, client.lastReq.ExpectedUpdatedAt) + } +} + +func TestCronUpdateAcceptsHarnessPrompt(t *testing.T) { + client := &recordingCronUpdateClient{} + tool := CronUpdateTool(client) + + _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","prompt":"check the updated deployment","expected_updated_at":"2026-07-30T23:00:00Z"}`)) + if err != nil { + t.Fatalf("cron_update: %v", err) + } + if client.lastReq.ExecConfig == nil || *client.lastReq.ExecConfig != `{"prompt":"check the updated deployment"}` { + t.Fatalf("prompt was not encoded as harness execution config: %#v", client.lastReq.ExecConfig) + } + properties, ok := tool.Definition.Parameters["properties"].(map[string]any) + if !ok || properties["prompt"] == nil { + t.Fatalf("cron_update schema does not expose prompt: %#v", tool.Definition.Parameters) + } + timeoutSchema, ok := properties["timeout_seconds"].(map[string]any) + if !ok || timeoutSchema["minimum"] != 1 { + t.Fatalf("cron_update schema does not require a positive timeout: %#v", properties["timeout_seconds"]) + } +} + +func TestCronUpdateRejectsNoOpAndInvalidInput(t *testing.T) { + tool := CronUpdateTool(&recordingCronUpdateClient{}) + for _, tc := range []struct { + name string + args string + want string + }{ + {name: "missing id", args: `{"schedule":"0 * * * *"}`, want: "id is required"}, + {name: "missing version", args: `{"id":"job-1","schedule":"0 * * * *"}`, want: "expected_updated_at is required"}, + {name: "no-op", args: `{"id":"job-1","expected_updated_at":"2026-07-31T00:00:00Z"}`, want: "at least one"}, + {name: "invalid timestamp", args: `{"id":"job-1","schedule":"0 * * * *","expected_updated_at":"later"}`, want: "expected_updated_at"}, + {name: "multiple execution inputs", args: `{"id":"job-1","command":"echo hi","prompt":"check it","expected_updated_at":"2026-07-31T00:00:00Z"}`, want: "only one"}, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := tool.Handler(context.Background(), json.RawMessage(tc.args)) + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("error = %v, want substring %q", err, tc.want) + } + }) + } +} + +func TestCronUpdateRejectsUnsafeTimeout(t *testing.T) { + client := &recordingCronUpdateClient{} + tool := CronUpdateTool(client) + for _, timeout := range []string{"0", "-1"} { + _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","timeout_seconds":`+timeout+`,"expected_updated_at":"2026-07-31T00:00:00Z"}`)) + if err == nil || !strings.Contains(err.Error(), "timeout_seconds must be positive") { + t.Fatalf("timeout %s error = %v, want actionable validation", timeout, err) + } + } +} + +func TestCronUpdateSchemaRequiresVersion(t *testing.T) { + tool := CronUpdateTool(&recordingCronUpdateClient{}) + required, ok := tool.Definition.Parameters["required"].([]string) + if !ok { + t.Fatal("required schema is not []string") + } + for _, field := range required { + if field == "expected_updated_at" { + return + } + } + t.Fatal("expected_updated_at must be required for cron_update") +} + +func TestCronUpdateReturnsClientError(t *testing.T) { + tool := CronUpdateTool(&recordingCronUpdateClient{err: errors.New("conflict")}) + _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","tags":"prod","expected_updated_at":"2026-07-31T00:00:00Z"}`)) + if err == nil || !strings.Contains(err.Error(), "conflict") { + t.Fatalf("error = %v, want client error", err) + } +} + +func TestCronPauseResumeRequireAndForwardExpectedVersion(t *testing.T) { + want := time.Date(2026, 7, 31, 12, 34, 56, 789, time.UTC) + for _, tc := range []struct { + name string + status string + tool func(tools.CronClient) tools.Tool + }{ + {name: "pause", status: "paused", tool: CronPauseTool}, + {name: "resume", status: "active", tool: CronResumeTool}, + } { + t.Run(tc.name, func(t *testing.T) { + client := &recordingCronUpdateClient{} + tool := tc.tool(client) + required, ok := tool.Definition.Parameters["required"].([]string) + if !ok || !slices.Contains(required, "expected_updated_at") { + t.Fatalf("required schema = %#v, want expected_updated_at", tool.Definition.Parameters["required"]) + } + if _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1"}`)); err == nil || !strings.Contains(err.Error(), "expected_updated_at is required") { + t.Fatalf("missing version error = %v", err) + } + if _, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","expected_updated_at":"not-a-time"}`)); err == nil || !strings.Contains(err.Error(), "RFC3339") { + t.Fatalf("invalid version error = %v", err) + } + args := `{"id":"job-1","expected_updated_at":"` + want.Format(time.RFC3339Nano) + `"}` + if _, err := tool.Handler(context.Background(), json.RawMessage(args)); err != nil { + t.Fatalf("versioned %s: %v", tc.name, err) + } + if client.lastReq.Status == nil || *client.lastReq.Status != tc.status { + t.Fatalf("status = %#v, want %q", client.lastReq.Status, tc.status) + } + if client.lastReq.ExpectedUpdatedAt == nil || !client.lastReq.ExpectedUpdatedAt.Equal(want) { + t.Fatalf("expected version = %#v, want %s", client.lastReq.ExpectedUpdatedAt, want) + } + }) + } +} + +func TestNewScopedCronClientIsIdempotent(t *testing.T) { + raw := &recordingCronUpdateClient{} + first := NewScopedCronClient(raw) + second := NewScopedCronClient(first) + if first != second { + t.Fatal("model registry wrapping an already-scoped cron client added a second wrapper") + } +} + +type recordingScopedDeleteClient struct { + tools.CronClient + job tools.CronJob + getID string + deleteID string + getScope cron.Scope + deleteScope cron.Scope +} + +func (c *recordingScopedDeleteClient) GetJob(ctx context.Context, id string) (tools.CronJob, error) { + c.getID = id + c.getScope, _ = cron.ScopeFromContext(ctx) + return c.job, nil +} + +func (c *recordingScopedDeleteClient) DeleteJob(ctx context.Context, id string) error { + c.deleteID = id + c.deleteScope, _ = cron.ScopeFromContext(ctx) + return nil +} + +func TestScopedCronClientUnversionedDeleteEnforcesScope(t *testing.T) { + metadata := tools.RunMetadata{TenantID: "tenant-a", ConversationID: "conversation-a", AgentID: "agent-a"} + ctx := context.WithValue(context.Background(), tools.ContextKeyRunMetadata, metadata) + wantScope := cron.Scope{TenantID: metadata.TenantID, ConversationID: metadata.ConversationID, AgentID: metadata.AgentID} + raw := &recordingScopedDeleteClient{job: tools.CronJob{ + ID: "job-a", + TenantID: metadata.TenantID, + ConversationID: metadata.ConversationID, + AgentID: metadata.AgentID, + }} + scoped := NewScopedCronClient(raw) + if err := scoped.DeleteJob(ctx, "job-a"); err != nil { + t.Fatalf("delete owned job: %v", err) + } + if raw.getID != "job-a" || raw.deleteID != "job-a" { + t.Fatalf("get/delete IDs = %q/%q, want job-a/job-a", raw.getID, raw.deleteID) + } + if raw.getScope != wantScope || raw.deleteScope != wantScope { + t.Fatalf("forwarded scopes = %#v/%#v, want %#v", raw.getScope, raw.deleteScope, wantScope) + } + + raw.job.TenantID = "tenant-b" + raw.deleteID = "" + if err := scoped.DeleteJob(ctx, "job-b"); !errors.Is(err, tools.ErrCronJobNotFound) { + t.Fatalf("cross-scope delete error = %v, want not found", err) + } + if raw.deleteID != "" { + t.Fatalf("cross-scope delete reached underlying mutation for %q", raw.deleteID) + } +} diff --git a/internal/harness/tools/deferred/deferred_test.go b/internal/harness/tools/deferred/deferred_test.go index 7aa09208..a84c4a41 100644 --- a/internal/harness/tools/deferred/deferred_test.go +++ b/internal/harness/tools/deferred/deferred_test.go @@ -9,7 +9,9 @@ import ( "net/url" "os" "path/filepath" + "strings" "testing" + "time" tools "go-agent-harness/internal/harness/tools" "go-agent-harness/internal/profiles" @@ -61,6 +63,9 @@ func (m *mockCronClient) GetJob(_ context.Context, id string) (tools.CronJob, er return tools.CronJob{ID: id}, nil } func (m *mockCronClient) DeleteJob(_ context.Context, id string) error { return nil } +func (m *mockCronClient) DeleteJobCAS(_ context.Context, id string, _ time.Time) error { + return nil +} func (m *mockCronClient) UpdateJob(_ context.Context, id string, req tools.CronUpdateJobRequest) (tools.CronJob, error) { return tools.CronJob{ID: id}, nil } @@ -455,6 +460,22 @@ func TestCronCreateTool_Definition(t *testing.T) { assertHasTags(t, tool, "cron") } +// TestCronCreateTool_SchemaIsAcceptedByOpenAICompatibleProviders guards the +// provider boundary: OpenAI-compatible function schemas require an object at +// the top level and reject composition keywords there. Shell-versus-harness +// argument pairing is enforced by CronCreateTool's handler instead. +func TestCronCreateTool_SchemaIsAcceptedByOpenAICompatibleProviders(t *testing.T) { + parameters := CronCreateTool(&mockCronClient{}).Definition.Parameters + if got := parameters["type"]; got != "object" { + t.Fatalf("cron_create schema type = %#v, want object", got) + } + for _, forbidden := range []string{"oneOf", "anyOf", "allOf", "enum", "const", "not"} { + if _, found := parameters[forbidden]; found { + t.Fatalf("cron_create schema has forbidden top-level %q: %#v", forbidden, parameters) + } + } +} + // TestCronCreateTool_Handler_Success verifies cron_create creates a job. func TestCronCreateTool_Handler_Success(t *testing.T) { tool := CronCreateTool(&mockCronClient{}) @@ -968,18 +989,16 @@ func TestStrPtr(t *testing.T) { // TestCronPauseTool_Handler_MissingID verifies cron_pause returns error when id is missing. func TestCronPauseTool_Handler_MissingID(t *testing.T) { tool := CronPauseTool(&mockCronClient{}) - _, err := tool.Handler(context.Background(), json.RawMessage(`{}`)) - // The handler parses args but "id" is empty string, not an unmarshal error. - // The UpdateJob mock doesn't fail on empty id, so we just check it returns without panic. - if err != nil { - t.Fatalf("unexpected error: %v", err) + _, err := tool.Handler(context.Background(), json.RawMessage(`{"expected_updated_at":"2026-03-08T11:00:00Z"}`)) + if err == nil || !strings.Contains(err.Error(), "id is required") { + t.Fatalf("error = %v, want id is required", err) } } // TestCronPauseTool_Handler_Success verifies cron_pause calls UpdateJob. func TestCronPauseTool_Handler_Success(t *testing.T) { tool := CronPauseTool(&mockCronClient{}) - result, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1"}`)) + result, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-1","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1002,7 +1021,7 @@ func TestCronPauseTool_Handler_InvalidJSON(t *testing.T) { // TestCronResumeTool_Handler_Success verifies cron_resume calls UpdateJob. func TestCronResumeTool_Handler_Success(t *testing.T) { tool := CronResumeTool(&mockCronClient{}) - result, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-2"}`)) + result, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"job-2","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1047,7 +1066,7 @@ func TestCancelDelayedCallbackTool_Handler_NotFound(t *testing.T) { // TestCronDeleteTool_Handler_Success verifies cron_delete calls DeleteJob. func TestCronDeleteTool_Handler_Success(t *testing.T) { tool := CronDeleteTool(&mockCronClient{}) - result, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"j1"}`)) + result, err := tool.Handler(context.Background(), json.RawMessage(`{"id":"j1","expected_updated_at":"2026-03-08T11:00:00Z"}`)) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/internal/harness/tools/descriptions/cron_create.md b/internal/harness/tools/descriptions/cron_create.md index 88a4353d..b4f80a29 100644 --- a/internal/harness/tools/descriptions/cron_create.md +++ b/internal/harness/tools/descriptions/cron_create.md @@ -1 +1,3 @@ -Create a RECURRING scheduled cron job. Cron jobs run repeatedly on a fixed schedule (e.g. every 5 minutes, every hour, daily at midnight). They are NOT one-shot timers — do NOT use cron_create if the user wants to run something once, after a delay, or at a single future time. For one-shot delayed execution, use bash (e.g. 'sleep 120 && command') instead. Write a script using write/bash tools first, then schedule it with cron. \ No newline at end of file +Create a RECURRING scheduled cron job. Cron jobs run repeatedly on a fixed schedule (e.g. every 5 minutes, every hour, daily at midnight). They are NOT one-shot timers — do NOT use cron_create if the user wants to run something once, after a delay, or at a single future time. For one-shot delayed execution that continues the current conversation, use `set_delayed_callback` instead. + +For a legacy shell job, provide a non-empty `command` and omit `execution_type` (or set `execution_type` to `shell`). For a conversational job that sends an assistant turn into the active conversation, set `execution_type` to `harness` and provide a non-empty `prompt`; do not provide `command`. The harness job's tenant, agent, conversation, job, and execution correlation fields come from immutable run metadata and are never accepted from model arguments. `timeout_seconds` is optional (default 30) but must be a positive integer when supplied. Invalid schedules and incomplete execution configurations are rejected with actionable errors. diff --git a/internal/harness/tools/descriptions/cron_delete.md b/internal/harness/tools/descriptions/cron_delete.md index 9e1244c7..2baacfd2 100644 --- a/internal/harness/tools/descriptions/cron_delete.md +++ b/internal/harness/tools/descriptions/cron_delete.md @@ -1 +1 @@ -Delete a cron job. \ No newline at end of file +Delete a cron job. First call cron_get, then pass the job ID and its updated_at value as expected_updated_at. A stale version is rejected instead of deleting a concurrently changed job. diff --git a/internal/harness/tools/descriptions/cron_get.md b/internal/harness/tools/descriptions/cron_get.md index be22dc1c..98b2e8d0 100644 --- a/internal/harness/tools/descriptions/cron_get.md +++ b/internal/harness/tools/descriptions/cron_get.md @@ -1 +1 @@ -Get details of a cron job including recent execution history. \ No newline at end of file +Get details of a cron job including recent execution history. Check `recent_executions_available` before interpreting an empty history; when retrieval fails, the job remains readable and `recent_executions_warning` explains that history is unavailable. diff --git a/internal/harness/tools/descriptions/cron_pause.md b/internal/harness/tools/descriptions/cron_pause.md index a7cf4d56..962415a3 100644 --- a/internal/harness/tools/descriptions/cron_pause.md +++ b/internal/harness/tools/descriptions/cron_pause.md @@ -1 +1 @@ -Pause a cron job. The job will not run until resumed. \ No newline at end of file +Pause a cron job. First call cron_get, then pass the job ID and its updated_at value as expected_updated_at. The job will not run until resumed; a stale version is rejected instead of overwriting a concurrent change. diff --git a/internal/harness/tools/descriptions/cron_resume.md b/internal/harness/tools/descriptions/cron_resume.md index d7d34a45..94ef4b88 100644 --- a/internal/harness/tools/descriptions/cron_resume.md +++ b/internal/harness/tools/descriptions/cron_resume.md @@ -1 +1 @@ -Resume a paused cron job. \ No newline at end of file +Resume a paused cron job. First call cron_get, then pass the job ID and its updated_at value as expected_updated_at. A stale version is rejected instead of overwriting a concurrent change. diff --git a/internal/harness/tools/descriptions/cron_update.md b/internal/harness/tools/descriptions/cron_update.md index 01a9c001..6fa5a685 100644 --- a/internal/harness/tools/descriptions/cron_update.md +++ b/internal/harness/tools/descriptions/cron_update.md @@ -1 +1 @@ -Edit an existing cron job in place: change its schedule, its command, its timeout, or its tags. Give the job ID plus only the fields you want changed; anything you omit is left as it is. Prefer this over deleting and recreating a job — recreating gives it a new ID and discards its execution history, which is the only record of whether it has been working. To pause or resume a job, use cron_pause or cron_resume instead of setting status here. \ No newline at end of file +Edit an existing cron job in place: change its schedule, shell command or harness prompt, timeout, or tags. `command` applies to shell jobs and `prompt` applies to harness jobs; this tool does not convert a job between execution types. First call cron_get, then give the job ID, its updated_at value as expected_updated_at, and only the fields you want changed; anything you omit is left as it is. Prefer this over deleting and recreating a job — recreating gives it a new ID and discards its execution history, which is the only record of whether it has been working. To pause or resume a job, use cron_pause or cron_resume instead of setting status here. diff --git a/internal/harness/tools/types.go b/internal/harness/tools/types.go index a60eae71..c82291ab 100644 --- a/internal/harness/tools/types.go +++ b/internal/harness/tools/types.go @@ -771,6 +771,7 @@ func SandboxScopeFromContext(ctx context.Context) (SandboxScope, bool) { // CronClient provides access to the cron scheduler daemon. var ErrCronJobNotFound = errors.New("cron job not found") +var ErrCronJobConflict = errors.New("cron job update conflict") type CronClient interface { CreateJob(ctx context.Context, req CronCreateJobRequest) (CronJob, error) @@ -778,6 +779,7 @@ type CronClient interface { GetJob(ctx context.Context, id string) (CronJob, error) UpdateJob(ctx context.Context, id string, req CronUpdateJobRequest) (CronJob, error) DeleteJob(ctx context.Context, id string) error + DeleteJobCAS(ctx context.Context, id string, expectedUpdatedAt time.Time) error ListExecutions(ctx context.Context, jobID string, limit, offset int) ([]CronExecution, error) Health(ctx context.Context) error } @@ -829,9 +831,10 @@ type CronCreateJobRequest struct { // CronUpdateJobRequest is the request for updating a cron job. type CronUpdateJobRequest struct { - Schedule *string `json:"schedule,omitempty"` - ExecConfig *string `json:"execution_config,omitempty"` - Status *string `json:"status,omitempty"` - TimeoutSec *int `json:"timeout_seconds,omitempty"` - Tags *string `json:"tags,omitempty"` + Schedule *string `json:"schedule,omitempty"` + ExecConfig *string `json:"execution_config,omitempty"` + Status *string `json:"status,omitempty"` + TimeoutSec *int `json:"timeout_seconds,omitempty"` + Tags *string `json:"tags,omitempty"` + ExpectedUpdatedAt *time.Time `json:"expected_updated_at,omitempty"` } diff --git a/internal/harness/tools_default.go b/internal/harness/tools_default.go index b67824d6..70020389 100644 --- a/internal/harness/tools_default.go +++ b/internal/harness/tools_default.go @@ -189,6 +189,18 @@ func NewDefaultRegistryWithOptions(workspaceRoot string, opts DefaultRegistryOpt askTimeout = 5 * time.Minute } + // Default registries are model-facing at every runtime assembly site: the + // daemon's top-level registry, worktree-isolated per-run rebuilds, and + // subagent registries all flow through this constructor. Scope cron here, + // once, instead of relying on each caller to remember a wrapper. Raw + // adapters remain on the operator HTTP/server paths and never pass through + // this boundary. NewScopedCronClient is idempotent for callers that already + // supplied the model wrapper. + var modelCronClient htools.CronClient + if opts.CronClient != nil { + modelCronClient = deferred.NewScopedCronClient(opts.CronClient) + } + httpClient := &http.Client{Timeout: 30 * time.Second} // Build shared resources @@ -222,7 +234,7 @@ func NewDefaultRegistryWithOptions(workspaceRoot string, opts DefaultRegistryOpt AgentRunner: opts.AgentRunner, SkillLister: opts.SkillLister, SkillVerifier: opts.SkillVerifier, - CronClient: opts.CronClient, + CronClient: modelCronClient, EnableTodos: true, // Code-intel/LSP tools (lsp_diagnostics, lsp_references, lsp_restart) are NOT // included in the default registry. They require a running language server and @@ -234,7 +246,7 @@ func NewDefaultRegistryWithOptions(workspaceRoot string, opts DefaultRegistryOpt EnableWebOps: true, ModelCatalog: opts.ModelCatalog, EnableSkills: opts.SkillLister != nil, - EnableCron: opts.CronClient != nil, + EnableCron: modelCronClient != nil, CallbackManager: opts.CallbackManager, EnableCallbacks: opts.CallbackManager != nil, Sourcegraph: opts.Sourcegraph, @@ -304,19 +316,19 @@ func NewDefaultRegistryWithOptions(workspaceRoot string, opts DefaultRegistryOpt // claim of success, which is the failure mode worth spending tool-list // space to avoid. // - // All six rather than the popular ones: a model that can list jobs but + // All eight rather than the popular ones: a model that can list jobs but // cannot pause one hits the same wall one step later, and splitting a // single capability across tiers is what produced this bug. - if buildOpts.EnableCron && opts.CronClient != nil { + if buildOpts.EnableCron && modelCronClient != nil { coreTools = append(coreTools, - deferred.CronCreateTool(opts.CronClient), - deferred.CronListTool(opts.CronClient), - deferred.CronGetTool(opts.CronClient), - deferred.CronDeleteTool(opts.CronClient), - deferred.CronPauseTool(opts.CronClient), - deferred.CronResumeTool(opts.CronClient), - deferred.CronUpdateTool(opts.CronClient), - deferred.CronHistoryTool(opts.CronClient), + deferred.CronCreateTool(modelCronClient), + deferred.CronListTool(modelCronClient), + deferred.CronGetTool(modelCronClient), + deferred.CronDeleteTool(modelCronClient), + deferred.CronPauseTool(modelCronClient), + deferred.CronResumeTool(modelCronClient), + deferred.CronUpdateTool(modelCronClient), + deferred.CronHistoryTool(modelCronClient), ) }