diff --git a/internal/orchestrator/event_loop.go b/internal/orchestrator/event_loop.go index 4817925..4fd01ff 100644 --- a/internal/orchestrator/event_loop.go +++ b/internal/orchestrator/event_loop.go @@ -232,10 +232,59 @@ func (o *Orchestrator) fireRetries(ctx context.Context, state State, now time.Ti // question comments. Used to identify and skip own comments when detecting user replies. const itervoxCommentPrefix = "🤖 **Agent needs your input**" +// findLatestItervoxComment returns the index and CreatedAt of the most recently +// created comment whose body starts with itervoxCommentPrefix. Returns -1 and +// the zero time when no such comment exists or none have a CreatedAt timestamp. +// Iterating the full slice (rather than a reverse-index walk) makes the result +// independent of tracker comment ordering — Linear's GraphQL connection returns +// comments newest-first by default, GitHub returns oldest-first. +func findLatestItervoxComment(comments []domain.Comment) (int, time.Time) { + latestIdx := -1 + var latestT time.Time + for i := range comments { + c := &comments[i] + if !strings.HasPrefix(c.Body, itervoxCommentPrefix) { + continue + } + if c.CreatedAt == nil { + continue + } + if latestIdx < 0 || c.CreatedAt.After(latestT) { + latestIdx = i + latestT = *c.CreatedAt + } + } + return latestIdx, latestT +} + +// firstReplyAfter returns the body of the first non-itervox comment whose +// CreatedAt is strictly after t, or "" if none. Comments with nil CreatedAt +// are conservatively skipped — without a timestamp we cannot prove they were +// posted after the question, so we must not treat them as replies. +func firstReplyAfter(comments []domain.Comment, t time.Time) string { + for i := range comments { + c := &comments[i] + if strings.HasPrefix(c.Body, itervoxCommentPrefix) { + continue + } + if c.CreatedAt == nil { + continue + } + if c.CreatedAt.After(t) { + return c.Body + } + } + return "" +} + // recoverInputRequired fetches the full issue detail (with comments) and checks // if the latest comment is an unresolved Itervox input-required question. // If so, returns an InputRequiredEntry reconstructed from the comment, // preventing a wasteful fresh dispatch. Returns nil if no recovery is needed. +// +// Identifies the unanswered question by CreatedAt timestamp, not array +// position, so the recovery path is robust to tracker comment ordering +// (e.g. Linear returns comments newest-first; GitHub returns oldest-first). func (o *Orchestrator) recoverInputRequired(ctx context.Context, issue domain.Issue) *InputRequiredEntry { detailed, err := o.tracker.FetchIssueDetail(ctx, issue.ID) if err != nil { @@ -246,25 +295,15 @@ func (o *Orchestrator) recoverInputRequired(ctx context.Context, issue domain.Is if len(detailed.Comments) == 0 { return nil } - // Walk comments in reverse to find the last Itervox question. - lastItervoxIdx := -1 - for i := len(detailed.Comments) - 1; i >= 0; i-- { - if strings.HasPrefix(detailed.Comments[i].Body, itervoxCommentPrefix) { - lastItervoxIdx = i - break - } - } - if lastItervoxIdx < 0 { + latestIdx, latestT := findLatestItervoxComment(detailed.Comments) + if latestIdx < 0 { return nil // no Itervox question comment found } - // Check if there's a non-Itervox comment after it (= user replied). - for i := lastItervoxIdx + 1; i < len(detailed.Comments); i++ { - if !strings.HasPrefix(detailed.Comments[i].Body, itervoxCommentPrefix) { - return nil // user already replied — safe to dispatch fresh - } + if firstReplyAfter(detailed.Comments, latestT) != "" { + return nil // user already replied — safe to dispatch fresh } // Extract the question context from the comment body. - body := detailed.Comments[lastItervoxIdx].Body + body := detailed.Comments[latestIdx].Body questionCtx := strings.TrimPrefix(body, itervoxCommentPrefix) questionCtx = strings.TrimSpace(questionCtx) // Strip the trailing instruction line. @@ -293,25 +332,14 @@ func (o *Orchestrator) checkTrackerReplies(ctx context.Context, state State) Sta "identifier", identifier, "error", err) continue } - // Find the last Itervox question comment and check for a reply after it. - lastItervoxIdx := -1 - for i := len(detailed.Comments) - 1; i >= 0; i-- { - if strings.HasPrefix(detailed.Comments[i].Body, itervoxCommentPrefix) { - lastItervoxIdx = i - break - } - } - if lastItervoxIdx < 0 { + // Identify the unanswered question by CreatedAt timestamp, not array + // position — Linear returns comments newest-first, GitHub oldest-first; + // timestamp comparison is correct for both. + _, latestT := findLatestItervoxComment(detailed.Comments) + if latestT.IsZero() { continue // no question comment found — wait } - // Look for a non-Itervox reply after the question. - var userReply string - for i := lastItervoxIdx + 1; i < len(detailed.Comments); i++ { - if !strings.HasPrefix(detailed.Comments[i].Body, itervoxCommentPrefix) { - userReply = detailed.Comments[i].Body - break - } - } + userReply := firstReplyAfter(detailed.Comments, latestT) if userReply == "" { continue // no reply yet } diff --git a/internal/orchestrator/event_loop_internal_test.go b/internal/orchestrator/event_loop_internal_test.go new file mode 100644 index 0000000..fc186f4 --- /dev/null +++ b/internal/orchestrator/event_loop_internal_test.go @@ -0,0 +1,300 @@ +package orchestrator + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/vnovick/itervox/internal/agent/agenttest" + "github.com/vnovick/itervox/internal/config" + "github.com/vnovick/itervox/internal/domain" + "github.com/vnovick/itervox/internal/tracker" +) + +// itervoxQuestionBody returns a body matching the format produced by the +// TerminalInputRequired handler in event_loop.go. +func itervoxQuestionBody(question string) string { + return itervoxCommentPrefix + "\n\n" + question + "\n\n---\n_Reply via the Itervox dashboard to continue._" +} + +// makeCommentWalkOrch builds a minimal Orchestrator wired to a MemoryTracker +// seeded with the given issue (which itself carries pre-populated Comments). +// The runner is a non-stalling FakeRunner so any goroutine spawned by +// checkTrackerReplies can complete cleanly. +func makeCommentWalkOrch(t *testing.T, issue domain.Issue) *Orchestrator { + t.Helper() + cfg := &config.Config{} + cfg.Tracker.ActiveStates = []string{"todo", "in-progress"} + cfg.Tracker.TerminalStates = []string{"done", "cancelled"} + cfg.Agent.MaxConcurrentAgents = 1 + cfg.Agent.MaxTurns = 1 + mt := tracker.NewMemoryTracker( + []domain.Issue{issue}, + cfg.Tracker.ActiveStates, + cfg.Tracker.TerminalStates, + ) + return New(cfg, mt, agenttest.NewFakeRunner(nil), nil) +} + +func ts(epochSec int64) *time.Time { + t := time.Unix(epochSec, 0).UTC() + return &t +} + +// --------------------------------------------------------------------------- +// recoverInputRequired — descending order (Linear-style) +// --------------------------------------------------------------------------- + +// Linear's GraphQL connection (`comments(first: 50, orderBy: createdAt)` in +// internal/tracker/linear/queries.go:53) returns comments in DESCENDING order +// (newest first). Walking the array by position assuming chronological order +// inverts the logic and misreads any non-itervox comment as a "user reply". +// This test pins the timestamp-based behaviour: with no real reply, an +// unanswered itervox question must be recoverable regardless of array order. +func TestRecoverInputRequired_DescendingOrder_NoUserReply(t *testing.T) { + t0 := ts(1000) // older — agent's own status comment + t2 := ts(2000) // newer — itervox question + + issue := domain.Issue{ + ID: "id1", + Identifier: "ENG-1", + Title: "T", + State: "in-progress", + Comments: []domain.Comment{ + // Linear order: newest first. + {Body: itervoxQuestionBody("Which approach: A or B?"), CreatedAt: t2, AuthorName: "itervox-bot"}, + {Body: "Status: looking into it", CreatedAt: t0, AuthorName: "agent"}, + }, + } + orch := makeCommentWalkOrch(t, issue) + + entry := orch.recoverInputRequired(context.Background(), issue) + require.NotNil(t, entry, "expected recovery: 🤖 question is the most-recent comment, no user reply after it") + assert.Equal(t, "id1", entry.IssueID) + assert.Equal(t, "ENG-1", entry.Identifier) + assert.Equal(t, "Which approach: A or B?", entry.Context) +} + +func TestRecoverInputRequired_DescendingOrder_WithUserReply(t *testing.T) { + t0 := ts(1000) // older + t2 := ts(2000) // itervox question + t3 := ts(3000) // newest — real user reply + + issue := domain.Issue{ + ID: "id1", + Identifier: "ENG-1", + Title: "T", + State: "in-progress", + Comments: []domain.Comment{ + {Body: "Use approach A.", CreatedAt: t3, AuthorName: "human"}, + {Body: itervoxQuestionBody("Which approach?"), CreatedAt: t2, AuthorName: "itervox-bot"}, + {Body: "Status update", CreatedAt: t0, AuthorName: "agent"}, + }, + } + orch := makeCommentWalkOrch(t, issue) + + entry := orch.recoverInputRequired(context.Background(), issue) + assert.Nil(t, entry, "expected nil: user reply (CreatedAt=3000) is later than itervox question (CreatedAt=2000)") +} + +// --------------------------------------------------------------------------- +// recoverInputRequired — ascending order (GitHub-style) +// --------------------------------------------------------------------------- + +func TestRecoverInputRequired_AscendingOrder_NoUserReply(t *testing.T) { + t0 := ts(1000) // older — agent's own comment + t2 := ts(2000) // newer — itervox question + + issue := domain.Issue{ + ID: "id1", + Identifier: "ENG-1", + Title: "T", + State: "in-progress", + Comments: []domain.Comment{ + // GitHub order: oldest first. + {Body: "Status: looking into it", CreatedAt: t0, AuthorName: "agent"}, + {Body: itervoxQuestionBody("Which approach?"), CreatedAt: t2, AuthorName: "itervox-bot"}, + }, + } + orch := makeCommentWalkOrch(t, issue) + + entry := orch.recoverInputRequired(context.Background(), issue) + require.NotNil(t, entry) + assert.Equal(t, "Which approach?", entry.Context) +} + +func TestRecoverInputRequired_AscendingOrder_WithUserReply(t *testing.T) { + t2 := ts(2000) // itervox question + t3 := ts(3000) // user reply + + issue := domain.Issue{ + ID: "id1", + Identifier: "ENG-1", + Title: "T", + State: "in-progress", + Comments: []domain.Comment{ + {Body: itervoxQuestionBody("Which approach?"), CreatedAt: t2, AuthorName: "itervox-bot"}, + {Body: "Use approach A.", CreatedAt: t3, AuthorName: "human"}, + }, + } + orch := makeCommentWalkOrch(t, issue) + + entry := orch.recoverInputRequired(context.Background(), issue) + assert.Nil(t, entry, "expected nil: user reply (CreatedAt=3000) is later than itervox question (CreatedAt=2000)") +} + +// --------------------------------------------------------------------------- +// recoverInputRequired — defensive: nil CreatedAt skipped, no question found +// --------------------------------------------------------------------------- + +func TestRecoverInputRequired_NoComments_ReturnsNil(t *testing.T) { + issue := domain.Issue{ID: "id1", Identifier: "ENG-1", Title: "T", State: "in-progress"} + orch := makeCommentWalkOrch(t, issue) + assert.Nil(t, orch.recoverInputRequired(context.Background(), issue)) +} + +func TestRecoverInputRequired_NilCreatedAt_DoesNotPanic(t *testing.T) { + t2 := ts(2000) + issue := domain.Issue{ + ID: "id1", + Identifier: "ENG-1", + Title: "T", + State: "in-progress", + Comments: []domain.Comment{ + {Body: "no timestamp comment", CreatedAt: nil, AuthorName: "agent"}, + {Body: itervoxQuestionBody("Which approach?"), CreatedAt: t2, AuthorName: "itervox-bot"}, + }, + } + orch := makeCommentWalkOrch(t, issue) + + entry := orch.recoverInputRequired(context.Background(), issue) + require.NotNil(t, entry) + assert.Equal(t, "Which approach?", entry.Context) +} + +// --------------------------------------------------------------------------- +// checkTrackerReplies — descending order preserves entry (regression test) +// --------------------------------------------------------------------------- + +// The 49-loop bug: with comments in Linear-descending order, today's index- +// based walk treats the older non-itervox comment as a user reply, deletes +// the InputRequiredIssues entry, and dispatches a "resumed" worker. This +// test pins the correct behaviour: state.InputRequiredIssues must be +// preserved when no genuine user reply is present. +func TestCheckTrackerReplies_DescendingOrder_PreservesEntry(t *testing.T) { + t0 := ts(1000) // agent's own pre-sentinel comment + t2 := ts(2000) // itervox question (most recent) + + issue := domain.Issue{ + ID: "id1", + Identifier: "ENG-1", + Title: "T", + State: "in-progress", + Comments: []domain.Comment{ + // Linear order: newest first. + {Body: itervoxQuestionBody("Which approach?"), CreatedAt: t2, AuthorName: "itervox-bot"}, + {Body: "Status: looking into it", CreatedAt: t0, AuthorName: "agent"}, + }, + } + orch := makeCommentWalkOrch(t, issue) + + cfg := &config.Config{} + cfg.Agent.MaxConcurrentAgents = 1 + state := NewState(cfg) + state.InputRequiredIssues["ENG-1"] = &InputRequiredEntry{ + IssueID: "id1", + Identifier: "ENG-1", + Context: "Which approach?", + QueuedAt: *t2, + } + + state = orch.checkTrackerReplies(context.Background(), state) + + _, present := state.InputRequiredIssues["ENG-1"] + assert.True(t, present, "InputRequiredIssues entry must NOT be deleted when no real user reply exists") + assert.Empty(t, state.Running, "no worker should be dispatched") + assert.Empty(t, state.Claimed, "no claim should be set") +} + +// --------------------------------------------------------------------------- +// checkTrackerReplies — descending order, real reply detected +// --------------------------------------------------------------------------- + +func TestCheckTrackerReplies_DescendingOrder_DetectsRealReply(t *testing.T) { + t0 := ts(1000) + t2 := ts(2000) + t3 := ts(3000) // real user reply, newest + + issue := domain.Issue{ + ID: "id1", + Identifier: "ENG-1", + Title: "T", + State: "in-progress", + Comments: []domain.Comment{ + {Body: "Use approach A.", CreatedAt: t3, AuthorName: "human"}, + {Body: itervoxQuestionBody("Which approach?"), CreatedAt: t2, AuthorName: "itervox-bot"}, + {Body: "Status update", CreatedAt: t0, AuthorName: "agent"}, + }, + } + orch := makeCommentWalkOrch(t, issue) + + // Cancel context after the test body so the spawned runWorkerWithResume + // goroutine can drain. sendExit will still deliver into o.events (capacity + // 64) without blocking even though no Run() loop is consuming. + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + cfg := &config.Config{} + cfg.Agent.MaxConcurrentAgents = 1 + state := NewState(cfg) + state.InputRequiredIssues["ENG-1"] = &InputRequiredEntry{ + IssueID: "id1", + Identifier: "ENG-1", + Context: "Which approach?", + QueuedAt: *t2, + } + + state = orch.checkTrackerReplies(ctx, state) + + _, present := state.InputRequiredIssues["ENG-1"] + assert.False(t, present, "InputRequiredIssues entry must be deleted when a real user reply is detected") + assert.Contains(t, state.Claimed, "id1", "issue should be claimed for the resumed worker") +} + +// --------------------------------------------------------------------------- +// checkTrackerReplies — ascending order baseline (GitHub-style happy path) +// --------------------------------------------------------------------------- + +func TestCheckTrackerReplies_AscendingOrder_PreservesEntry(t *testing.T) { + t0 := ts(1000) + t2 := ts(2000) + + issue := domain.Issue{ + ID: "id1", + Identifier: "ENG-1", + Title: "T", + State: "in-progress", + Comments: []domain.Comment{ + {Body: "Status: looking into it", CreatedAt: t0, AuthorName: "agent"}, + {Body: itervoxQuestionBody("Which approach?"), CreatedAt: t2, AuthorName: "itervox-bot"}, + }, + } + orch := makeCommentWalkOrch(t, issue) + + cfg := &config.Config{} + cfg.Agent.MaxConcurrentAgents = 1 + state := NewState(cfg) + state.InputRequiredIssues["ENG-1"] = &InputRequiredEntry{ + IssueID: "id1", + Identifier: "ENG-1", + Context: "Which approach?", + QueuedAt: *t2, + } + + state = orch.checkTrackerReplies(context.Background(), state) + + _, present := state.InputRequiredIssues["ENG-1"] + assert.True(t, present, "InputRequiredIssues entry must be preserved on the GitHub-style ascending path too") +}