diff --git a/server/internal/daemon/execenv/runtime_config.go b/server/internal/daemon/execenv/runtime_config.go index 77769e8a9..dae6964b0 100644 --- a/server/internal/daemon/execenv/runtime_config.go +++ b/server/internal/daemon/execenv/runtime_config.go @@ -388,7 +388,8 @@ func buildMetaSkillContent(provider string, ctx TaskContextForEnv) string { // Sub-issue Protocol (PR #2918) that still belongs in the brief. The // parent-notification guidance was dropped in MUL-2538: the platform // now posts a system comment on the parent itself when a child enters - // `done`, and the agent has nothing to do or avoid on that path. + // a completed state (`in_review` after MUL-2766, or `done`), and the + // agent has nothing to do or avoid on that path. // Section is skipped for chat, quick-create, and run-only autopilot // runs (no parent/child semantics there). if ctx.IssueID != "" && ctx.ChatSessionID == "" && ctx.QuickCreatePrompt == "" && ctx.AutopilotRunID == "" { diff --git a/server/internal/handler/issue_child_done.go b/server/internal/handler/issue_child_done.go index d9d1ca63b..69aa5ac1d 100644 --- a/server/internal/handler/issue_child_done.go +++ b/server/internal/handler/issue_child_done.go @@ -13,16 +13,25 @@ import ( ) // notifyParentOfChildDone posts a top-level system comment on the parent -// issue when a child issue transitions from non-done into done. This replaces -// the agent-prompt rule that previously made child agents post the -// notification themselves (PR #2918 user feedback — the agent rule caused -// self-mention loops, planner ping-pong, and accidental `MUL-` prefix -// hardcoding because the agent did not always know the workspace prefix). +// issue when a child issue transitions from a non-completed state into a +// completed one. A child is "completed" once it reaches `in_review` (the +// child agent has finished its work and is waiting for the parent's review) +// or `done` (the work is accepted). Both transitions fire the notification; +// the in_review case was MUL-2766 — squads were getting stuck because the +// child agent followed the runtime workflow (`in_review` when done, leader +// promotes to `done` after review) and the parent never woke up. The +// `in_review` ↔ `done` transition itself is a no-op (the parent has already +// been informed). This replaces the agent-prompt rule that previously made +// child agents post the notification themselves (PR #2918 user feedback — +// the agent rule caused self-mention loops, planner ping-pong, and +// accidental `MUL-` prefix hardcoding because the agent did not always know +// the workspace prefix). // // Guards on whether the comment fires at all: -// - prev.Status must not already be "done" (idempotent — repeat saves of -// done do not re-fire; only the transition fires) -// - issue.Status must be "done" +// - prev.Status must not already be a completed state (idempotent — once +// the parent has been told the child is in_review, moving it to done +// does not re-fire, and vice versa) +// - issue.Status must be a completed state (`in_review` or `done`) // - issue.ParentIssueID must be set // - parent must not be "done" or "cancelled" — the parent is already // closed and a notification has no follow-up to drive @@ -52,7 +61,7 @@ func (h *Handler) notifyParentOfChildDone(ctx context.Context, prev, issue db.Is if !issue.ParentIssueID.Valid { return } - if prev.Status == "done" || issue.Status != "done" { + if isChildCompletedStatus(prev.Status) || !isChildCompletedStatus(issue.Status) { return } parent, err := h.Queries.GetIssue(ctx, issue.ParentIssueID) @@ -83,10 +92,11 @@ func (h *Handler) notifyParentOfChildDone(ctx context.Context, prev, issue db.Is // agent the workspace lost track of, etc.). mentionPrefix := h.buildParentAssigneeMention(ctx, parent) - content := fmt.Sprintf( - "%sSub-issue [%s](mention://issue/%s) — \"%s\" — is done. Before promoting any waiting `backlog` sub-issue, read each sibling's description and only promote items whose stated dependencies are already satisfied — do not rely on this parent's higher-level breakdown alone. If a sibling's description conflicts with that breakdown (e.g. it lists a prerequisite the parent treats as parallel), do NOT change its status — leave it `backlog` and post a comment to confirm first.", - mentionPrefix, identifier, childID, title, - ) + content := childCompletedSystemCommentContent(issue.Status, mentionPrefix, identifier, childID, title) + if content == "" { + // Defensive: isChildCompletedStatus already filtered the status above. + return + } // author_type='system', author_id=zero UUID. The zero UUID is a valid 16 // byte value and the column is NOT NULL; frontend code should branch on @@ -125,6 +135,48 @@ func (h *Handler) notifyParentOfChildDone(ctx context.Context, prev, issue db.Is h.dispatchParentAssigneeTrigger(ctx, parent, issue, comment) } +// isChildCompletedStatus reports whether a child issue status represents +// "work handed back to the parent". Both `in_review` (waiting for leader +// review) and `done` (accepted) count: the parent's assignee should be +// woken on the transition INTO either, and transitions between them +// (in_review → done, done → in_review) are no-ops. See MUL-2766 for the +// in_review case; autopilot already treats these two statuses as a single +// completion signal (see service/autopilot.go), and this helper aligns the +// child-done notification with that convention. +func isChildCompletedStatus(status string) bool { + return status == "in_review" || status == "done" +} + +// childCompletedSystemCommentContent renders the system-comment body for a +// child-completed notification. The wording differs by status because the +// follow-up the parent assignee should take is different: +// +// - `in_review` is a coordination signal — the leader must inspect the +// work and either accept it (move child to `done`) or send it back +// (move child to `in_progress` with comment-described changes). +// Promoting backlog siblings is premature until the review lands. +// - `done` is a completion signal — the work is accepted, so the +// follow-up shifts to advancing the plan: promoting the next ready +// backlog sibling. The MUL-2538 promotion guardrails apply. +// +// Returns "" when the status is not a recognised completed state; callers +// should treat that as "do not post". +func childCompletedSystemCommentContent(status, mentionPrefix, identifier, childID, title string) string { + switch status { + case "in_review": + return fmt.Sprintf( + "%sSub-issue [%s](mention://issue/%s) — \"%s\" — is ready for review. Inspect the work the child reported, then either accept it by moving the child to `done`, or send it back by moving the child to `in_progress` with a comment describing the requested changes. Do NOT promote any waiting `backlog` sub-issue until the review lands.", + mentionPrefix, identifier, childID, title, + ) + case "done": + return fmt.Sprintf( + "%sSub-issue [%s](mention://issue/%s) — \"%s\" — is done. Before promoting any waiting `backlog` sub-issue, read each sibling's description and only promote items whose stated dependencies are already satisfied — do not rely on this parent's higher-level breakdown alone. If a sibling's description conflicts with that breakdown (e.g. it lists a prerequisite the parent treats as parallel), do NOT change its status — leave it `backlog` and post a comment to confirm first.", + mentionPrefix, identifier, childID, title, + ) + } + return "" +} + // sanitizeChildTitleForSystemComment removes mention-style markdown from a // child issue's title before it is embedded into the parent's system // comment. Smuggled mentions are already harmless on the listener path diff --git a/server/internal/handler/issue_child_done_test.go b/server/internal/handler/issue_child_done_test.go index 61da70b79..8599cfce0 100644 --- a/server/internal/handler/issue_child_done_test.go +++ b/server/internal/handler/issue_child_done_test.go @@ -154,6 +154,80 @@ func TestChildDoneNotifiesParent(t *testing.T) { } } +// TestChildInReviewNotifiesParent — MUL-2766. The runtime workflow tells +// child agents to move their issue to `in_review` when they finish (the +// human/leader then promotes to `done`). Before the fix, only the +// `done` transition fired the parent notification, so squads were getting +// stuck: leader assigned work out, child agents moved to in_review, parent +// leader was never woken. This test asserts that the in_review transition +// produces exactly one top-level system comment on the parent and that the +// body uses the in_review-flavoured wording (review-action verb, not the +// done-flavoured "Before promoting any waiting backlog sub-issue" clause). +func TestChildInReviewNotifiesParent(t *testing.T) { + fx := newChildDoneFixture(t, "in_progress") + + updateChildStatus(t, fx.child.ID, "in_review") + + if got := countSystemCommentsOn(t, fx.parent.ID); got != 1 { + t.Fatalf("expected exactly 1 system comment on parent after in_review, got %d", got) + } + content, _, parentNull, typeStr := systemCommentOn(t, fx.parent.ID) + if !parentNull { + t.Errorf("system comment must be top-level (parent_id IS NULL)") + } + if typeStr != "system" { + t.Errorf("system comment type should be 'system', got %q", typeStr) + } + if !strings.Contains(content, "is ready for review") { + t.Errorf("expected in_review-flavoured wording, got: %s", content) + } + if strings.Contains(content, "Before promoting any waiting `backlog`") { + t.Errorf("in_review comment should NOT carry the done-flavoured promote-backlog clause, got: %s", content) + } + if !strings.Contains(content, fx.child.Identifier) { + t.Errorf("expected comment to contain child identifier %q, got: %s", fx.child.Identifier, content) + } + if !strings.Contains(content, "mention://issue/"+fx.child.ID) { + t.Errorf("expected mention://issue/ link in comment, got: %s", content) + } +} + +// TestChildInReviewThenDoneDoesNotRefire — the parent has already been +// woken on in_review; promoting the child from in_review to done is the +// leader accepting the work, not a new completion event, and must not +// produce a second system comment. +func TestChildInReviewThenDoneDoesNotRefire(t *testing.T) { + fx := newChildDoneFixture(t, "in_progress") + + updateChildStatus(t, fx.child.ID, "in_review") + if got := countSystemCommentsOn(t, fx.parent.ID); got != 1 { + t.Fatalf("after in_review: expected 1 comment, got %d", got) + } + + updateChildStatus(t, fx.child.ID, "done") + if got := countSystemCommentsOn(t, fx.parent.ID); got != 1 { + t.Fatalf("after in_review→done: expected still 1 comment (no re-fire), got %d", got) + } +} + +// TestChildDoneAfterInReviewSendBackFiresAgain — the leader-driven +// "send back for changes" path: child reaches in_review (fires once), +// leader sends it back by moving to in_progress, child completes again +// and lands in_review (or done). The second completion is a real new +// event and must fire a second notification — without this, the leader +// would never be woken for the re-review. +func TestChildDoneAfterInReviewSendBackFiresAgain(t *testing.T) { + fx := newChildDoneFixture(t, "in_progress") + + updateChildStatus(t, fx.child.ID, "in_review") + updateChildStatus(t, fx.child.ID, "in_progress") + updateChildStatus(t, fx.child.ID, "in_review") + + if got := countSystemCommentsOn(t, fx.parent.ID); got != 2 { + t.Fatalf("expected 2 system comments after send-back + re-review cycle, got %d", got) + } +} + // TestChildDoneNotificationIsIdempotent — re-saving an already-done child // must NOT fire a second notification. UpdateIssue is called with the same // status='done' twice; only the first call is a transition and should