Skip to content

Commit 0d5c6b0

Browse files
committed
feat(orchestration): platform settles the comment + sub-issue on iteration done — 👀→✅/❌ + In Review (aws-samples#247 UX.21)
User-caught, three symptoms one cause: after a comment-iteration finishes, (1) the trigger comment stayed on 👀 forever (never ✅), (2) the sub-issue state flapped In Progress/In Review, and (3) the panel showed ✅ (correct, reads child_status) while the sub-issue said In Progress — three views disagreeing. Root cause: the reconciler posted the threaded ✅/❌ reply but never settled the comment REACTION or the sub-issue STATE; state was delegated to the AGENT via a prompt instruction (non-deterministic, raced, got stuck). Fix: the platform now owns settlement (as it already does for the parent epic's reaction/state). In replyToIterationComment, after the reply: - swap the TRIGGER comment 👀 → ✅ (success) / ❌ (failure) so the comment reads done at a glance, not just the threaded reply. - on success, advance the SUB-ISSUE to In Review (PR updated & open, awaiting human merge — same convention the epic uses; user's choice). On failure, leave the state (never demote). The reaction/state target the actual trigger comment + the iterated sub-issue, so a parent-routed comment settles the parent's comment + the footer sub-issue correctly. Gated once-only by the existing ack_replied_at claim; both helpers are idempotent (re-converge to one marker / skip if already in target state). New helper swapCommentReaction (mirrors swapIssueReaction but on a COMMENT — queries comment(id){reactions}, deletes stale bgagent markers, adds the target; never touches a human reaction). Tests: helper (swap/idempotent/ human-safe/no-token) + reconciler success→✅+In Review, failure→❌+no transition. 80 green across linear-feedback + reconciler suites.
1 parent d817ab8 commit 0d5c6b0

4 files changed

Lines changed: 124 additions & 1 deletion

File tree

cdk/src/handlers/orchestration-reconciler.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ import {
4747
import type { DynamoDBRecord, DynamoDBStreamEvent } from 'aws-lambda';
4848
import { createTaskCore } from './shared/create-task-core';
4949
import { renderFailureReply } from './shared/failure-reply';
50-
import { postIssueComment, replyToComment } from './shared/linear-feedback';
50+
import { EMOJI_FAILURE, EMOJI_SUCCESS, postIssueComment, replyToComment, swapCommentReaction, transitionIssueState } from './shared/linear-feedback';
5151
import { logger } from './shared/logger';
5252
import { isIntegrationNode } from './shared/orchestration-integration-node';
5353
import { ORCH_LOG } from './shared/orchestration-log-events';
@@ -682,6 +682,21 @@ async function replyToIterationComment(
682682
// tasks created before UX.19 (no triggerCommentIssueId persisted).
683683
const replyIssueId = evt.triggerCommentIssueId ?? changedSubIssueId;
684684
await replyToComment(ctx, replyIssueId, commentId, body);
685+
686+
// #247 UX.21: settle the comment + sub-issue so all three views agree (panel
687+
// row, sub-issue state, comment reaction) — the platform owns this, not the
688+
// agent (whose prompt-driven state-setting flapped In Progress/In Review).
689+
// - swap the TRIGGER comment's 👀 → ✅ (success) / ❌ (failure), so the
690+
// comment itself reads done at a glance, not just the threaded reply.
691+
// - on success, advance the SUB-ISSUE to In Review (its PR is updated &
692+
// open, awaiting human merge — same convention the epic uses). On
693+
// failure, leave the state (the ❌ + reply convey it). Never demote.
694+
// Best-effort + idempotent (the ack_replied_at claim above already gates this
695+
// to once per iteration; swapCommentReaction/transition re-converge anyway).
696+
await swapCommentReaction(ctx, commentId, succeeded ? EMOJI_SUCCESS : EMOJI_FAILURE);
697+
if (succeeded) {
698+
await transitionIssueState(ctx, changedSubIssueId, 'started', ['In Review']);
699+
}
685700
}
686701

687702
/** Build the ✅ ack reply, linking the (re-pushed) PR when resolvable. */

cdk/src/handlers/shared/linear-feedback.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,13 @@ query IssueReactions($issueId: String!) {
126126
}
127127
`.trim();
128128

129+
/** Read a COMMENT's reactions (id + emoji) — to swap the comment's bgagent marker (#247 UX.21). */
130+
const COMMENT_REACTIONS_QUERY = `
131+
query CommentReactions($commentId: String!) {
132+
comment(id: $commentId) { reactions { id emoji } }
133+
}
134+
`.trim();
135+
129136
/**
130137
* The bgagent status-marker emojis we manage on the PARENT epic. Mirrors
131138
* ``_BGAGENT_EMOJIS`` in ``agent/src/linear_reactions.py``. Only these are
@@ -434,6 +441,41 @@ export async function swapIssueReaction(
434441
return (await graphqlRequest(token, REACTION_CREATE_MUTATION, { issueId, emoji })).ok;
435442
}
436443

444+
/**
445+
* Swap the bgagent status marker on a COMMENT (👀 → ✅/❌), so the trigger
446+
* comment shows ONE marker reflecting the outcome — mirrors
447+
* {@link swapIssueReaction} but on a comment (#247 UX.21). The 👀 lands at
448+
* receipt ({@link reactToComment}); when the iteration settles we swap it for
449+
* ✅ (success) / ❌ (failure) so the comment itself reads done at a glance, not
450+
* just the threaded reply. Queries the comment's reactions, deletes every
451+
* bgagent marker except the target, adds the target if absent. Only bgagent
452+
* emojis (👀/✅/❌) are removed — a human's reaction is never touched.
453+
* Idempotent (a reconciler redelivery re-converges to the same single marker).
454+
* Best-effort; returns true if the target marker is present afterwards.
455+
*/
456+
export async function swapCommentReaction(
457+
ctx: LinearFeedbackContext,
458+
commentId: string,
459+
emoji: string,
460+
): Promise<boolean> {
461+
const token = await resolveToken(ctx);
462+
if (!token) return false;
463+
464+
const data = await graphqlData(token, COMMENT_REACTIONS_QUERY, { commentId });
465+
const reactions = ((data?.comment as { reactions?: Array<{ id: string; emoji: string }> } | undefined)?.reactions) ?? [];
466+
467+
let targetPresent = false;
468+
for (const r of reactions) {
469+
if (r.emoji === emoji) { targetPresent = true; continue; }
470+
if (BGAGENT_EMOJIS.has(r.emoji)) {
471+
await graphqlRequest(token, REACTION_DELETE_MUTATION, { id: r.id });
472+
}
473+
}
474+
475+
if (targetPresent) return true;
476+
return (await graphqlRequest(token, REACTION_CREATE_ON_COMMENT_MUTATION, { commentId, emoji })).ok;
477+
}
478+
437479
/**
438480
* Convenience: post a feedback comment **and** drop a ❌ reaction in one call.
439481
* Both calls run in parallel; both are best-effort. Returns void — callers

cdk/test/handlers/orchestration-reconciler.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,14 @@ jest.mock('../../src/handlers/shared/create-task-core', () => ({
3737
const postIssueCommentMock = jest.fn();
3838
const upsertStatusCommentMock = jest.fn();
3939
const swapIssueReactionMock = jest.fn();
40+
const swapCommentReactionMock = jest.fn();
4041
const transitionIssueStateMock = jest.fn();
4142
const replyToCommentMock = jest.fn();
4243
jest.mock('../../src/handlers/shared/linear-feedback', () => ({
4344
postIssueComment: (...args: unknown[]) => postIssueCommentMock(...args),
4445
upsertStatusComment: (...args: unknown[]) => upsertStatusCommentMock(...args),
4546
swapIssueReaction: (...args: unknown[]) => swapIssueReactionMock(...args),
47+
swapCommentReaction: (...args: unknown[]) => swapCommentReactionMock(...args),
4648
transitionIssueState: (...args: unknown[]) => transitionIssueStateMock(...args),
4749
replyToComment: (...args: unknown[]) => replyToCommentMock(...args),
4850
EMOJI_SUCCESS: 'white_check_mark',
@@ -665,6 +667,7 @@ describe('orchestration-reconciler handler — A6 iteration ack reply (#247 UX.3
665667
postIssueCommentMock.mockReset().mockResolvedValue(true);
666668
upsertStatusCommentMock.mockReset().mockResolvedValue('panel-cmt-1');
667669
swapIssueReactionMock.mockReset().mockResolvedValue(true);
670+
swapCommentReactionMock.mockReset().mockResolvedValue(true);
668671
transitionIssueStateMock.mockReset().mockResolvedValue(true);
669672
replyToCommentMock.mockReset().mockResolvedValue('reply-1');
670673
});
@@ -695,6 +698,10 @@ describe('orchestration-reconciler handler — A6 iteration ack reply (#247 UX.3
695698
expect(issueId).toBe('A'); // the sub-issue the comment lives on
696699
expect(parentCommentId).toBe('human-cmt-1');
697700
expect(body).toMatch(/^ Updated PR #\d+\./);
701+
// #247 UX.21: the trigger comment's 👀 swaps to ✅, and the sub-issue
702+
// advances to In Review (platform-owned settle, not agent-flapped).
703+
expect(swapCommentReactionMock).toHaveBeenCalledWith(expect.anything(), 'human-cmt-1', 'white_check_mark');
704+
expect(transitionIssueStateMock).toHaveBeenCalledWith(expect.anything(), 'A', 'started', ['In Review']);
698705
});
699706

700707
test('#247 UX.19: a PARENT-routed iteration replies on the PARENT issue, not the sub-issue', async () => {
@@ -734,6 +741,10 @@ describe('orchestration-reconciler handler — A6 iteration ack reply (#247 UX.3
734741
expect(body).toMatch(/reply with guidance/i);
735742
// A failed iteration still does not cascade onto dependents.
736743
expect(createTaskCoreMock).not.toHaveBeenCalled();
744+
// #247 UX.21: the trigger comment's 👀 swaps to ❌, but the sub-issue state
745+
// is LEFT in place on failure (the ❌ + reply convey it; never demote).
746+
expect(swapCommentReactionMock).toHaveBeenCalledWith(expect.anything(), 'human-cmt-1', 'x');
747+
expect(transitionIssueStateMock).not.toHaveBeenCalled();
737748
});
738749

739750
test('COMPLETED-but-build-failed iteration → ❌ build/test reply pointing at PR checks (UX.5)', async () => {

cdk/test/handlers/shared/linear-feedback.test.ts

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import {
3333
reactToComment,
3434
replyToComment,
3535
reportIssueFailure,
36+
swapCommentReaction,
3637
swapIssueReaction,
3738
transitionIssueState,
3839
upsertStatusComment,
@@ -339,6 +340,60 @@ describe('linear-feedback', () => {
339340
});
340341
});
341342

343+
describe('swapCommentReaction (#247 UX.21 — settle the trigger comment 👀→✅/❌)', () => {
344+
const commentReactionsResp = (rs: Array<{ id: string; emoji: string }>) =>
345+
jsonResponse({ data: { comment: { reactions: rs } } });
346+
347+
test('👀 on the comment → deletes it and adds ✅ (on the COMMENT, not the issue)', async () => {
348+
fetchMock
349+
.mockResolvedValueOnce(commentReactionsResp([{ id: 'r-eyes', emoji: 'eyes' }]))
350+
.mockResolvedValueOnce(jsonResponse({ data: { reactionDelete: { success: true } } }))
351+
.mockResolvedValueOnce(jsonResponse({ data: { reactionCreate: { success: true } } }));
352+
const ok = await swapCommentReaction(CTX, 'comment-77', 'white_check_mark');
353+
expect(ok).toBe(true);
354+
// query targets the COMMENT
355+
expect(JSON.parse(fetchMock.mock.calls[0][1].body).variables).toEqual({ commentId: 'comment-77' });
356+
// delete the stale 👀
357+
expect(JSON.parse(fetchMock.mock.calls[1][1].body).variables).toEqual({ id: 'r-eyes' });
358+
// create the ✅ via reactionCreate(commentId)
359+
const createVars = JSON.parse(fetchMock.mock.calls[2][1].body).variables;
360+
expect(createVars).toEqual({ commentId: 'comment-77', emoji: 'white_check_mark' });
361+
});
362+
363+
test('target already present → no re-create (idempotent under redelivery)', async () => {
364+
fetchMock
365+
.mockResolvedValueOnce(commentReactionsResp([
366+
{ id: 'r-eyes', emoji: 'eyes' },
367+
{ id: 'r-check', emoji: 'white_check_mark' },
368+
]))
369+
.mockResolvedValueOnce(jsonResponse({ data: { reactionDelete: { success: true } } }));
370+
const ok = await swapCommentReaction(CTX, 'comment-77', 'white_check_mark');
371+
expect(ok).toBe(true);
372+
expect(fetchMock).toHaveBeenCalledTimes(2); // query + delete 👀; ✅ already present
373+
});
374+
375+
test('never deletes a human reaction on the comment', async () => {
376+
fetchMock
377+
.mockResolvedValueOnce(commentReactionsResp([
378+
{ id: 'r-eyes', emoji: 'eyes' },
379+
{ id: 'r-heart', emoji: 'heart' }, // human — must survive
380+
]))
381+
.mockResolvedValueOnce(jsonResponse({ data: { reactionDelete: { success: true } } }))
382+
.mockResolvedValueOnce(jsonResponse({ data: { reactionCreate: { success: true } } }));
383+
await swapCommentReaction(CTX, 'comment-77', 'x');
384+
const deletedIds = fetchMock.mock.calls
385+
.filter((c) => JSON.parse(c[1].body).query.includes('reactionDelete'))
386+
.map((c) => JSON.parse(c[1].body).variables.id);
387+
expect(deletedIds).toEqual(['r-eyes']); // never r-heart
388+
});
389+
390+
test('no token → false, no fetch', async () => {
391+
resolveLinearOauthTokenMock.mockResolvedValueOnce(null);
392+
expect(await swapCommentReaction(CTX, 'comment-77', 'eyes')).toBe(false);
393+
expect(fetchMock).not.toHaveBeenCalled();
394+
});
395+
});
396+
342397
describe('upsertStatusComment (#3 live status block)', () => {
343398
test('no existing id → creates a comment and returns the new id', async () => {
344399
fetchMock.mockResolvedValueOnce(

0 commit comments

Comments
 (0)