diff --git a/backend/apps/agents/agents.py b/backend/apps/agents/agents.py index 779d025f..ccb81c5e 100644 --- a/backend/apps/agents/agents.py +++ b/backend/apps/agents/agents.py @@ -377,6 +377,9 @@ async def close_session(session_id: str): p_s = agent_manager.sessions.get(session_id) if p_s is not None: p_s.ended_by_user = True + # Only THIS route means "the user put it away". agent_manager.close_session is also called by + # the workflow executor for bookkeeping, so the flag belongs at the door, not in the helper. + p_s.dismissed_by_user = True try: await agent_manager.close_session(session_id) except ValueError as e: diff --git a/backend/apps/agents/core/models.py b/backend/apps/agents/core/models.py index 9cf6b99d..06e0015b 100644 --- a/backend/apps/agents/core/models.py +++ b/backend/apps/agents/core/models.py @@ -100,6 +100,11 @@ class AgentSession(BaseModel): branch: Optional[str] = None created_at: datetime = Field(default_factory=datetime.now) closed_at: Optional[datetime] = None + # Set ONLY by the close route, i.e. a person dismissing the card. `closed_at` cannot carry this: + # the workflow executor closes the agent session at the end of every step (so the run sorts into + # chat history) including one the user stopped, so it means "this run is over", not "put it away". + # One field per meaning, or a card the user stopped to READ disappears on them (ENG-421). + dismissed_by_user: bool = False # Wall-clock of the first stream event so resumed sessions can show "first response at HH:MM" without rescan. first_response_at: Optional[datetime] = None # HITL approval log: {tool, behavior, decision_ms} per entry. diff --git a/backend/tests/test_read_agent_work.py b/backend/tests/test_read_agent_work.py index ba66175f..cde53649 100644 --- a/backend/tests/test_read_agent_work.py +++ b/backend/tests/test_read_agent_work.py @@ -130,3 +130,21 @@ def test_the_route_reads_a_session_that_is_only_on_DISK(): assert "resume_session" in body, "a read tool that 404s on a stored session defeats its purpose" assert body.index("agent_manager.get_session") < body.index("resume_session"), \ "memory first, disk second: resuming every read would be a pointless load" + + +def test_only_the_close_route_marks_a_session_dismissed(): + """ENG-421, found on the packaged build: `closed_at` cannot mean "the user put this away", + because the workflow executor stamps it at the end of every step including a stopped one. Stop + and Close were therefore indistinguishable and a card the user stopped to READ vanished ~2s + later. The flag belongs at the close DOOR, not in agent_manager.close_session, which the + executor also calls.""" + src = open("backend/apps/agents/agents.py", encoding="utf-8").read() + i_close = src.index("async def close_session(") + i_stop = src.index("async def stop_agent(") + close_body = src[i_close:i_close + 700] + stop_body = src[i_stop:i_stop + 700] + assert "dismissed_by_user = True" in close_body, "Close must say the user put it away" + assert "dismissed_by_user" not in stop_body, "Stop must NOT read as a dismissal" + helper = open("backend/apps/agents/manager/SessionControl.py", encoding="utf-8").read() + assert "dismissed_by_user" not in helper, \ + "the executor calls this helper for bookkeeping; a flag here would dismiss stopped runs again" diff --git a/frontend/src/shared/state/deservesCanvasCard.test.ts b/frontend/src/shared/state/deservesCanvasCard.test.ts index b3ff2ae3..b7d4a92f 100644 --- a/frontend/src/shared/state/deservesCanvasCard.test.ts +++ b/frontend/src/shared/state/deservesCanvasCard.test.ts @@ -61,10 +61,12 @@ test('a run that ended on its own still despawns', () => { }); test('Close still dismisses it, even though Close also sets ended_by_user', () => { - // Both routes stamp ended_by_user; only Close stamps closed_at, which is what separates them. + // Both routes stamp ended_by_user, and `closed_at` cannot separate them: the workflow executor + // stamps it at the end of EVERY step, including a stopped one. `dismissed_by_user` is the fact + // that only the close route writes, so it is what tells "put it away" from "this run is over". assert.equal(deservesCanvasCard({ mode: 'agent', workflow_run_id: 'r1', status: 'stopped', - ended_by_user: true, closed_at: '2026-08-28T00:00:00Z', + ended_by_user: true, closed_at: '2026-08-28T00:00:00Z', dismissed_by_user: true, }), false); }); @@ -80,3 +82,29 @@ test('a live run is unaffected either way', () => { test('a user-launched chat never depends on any of this', () => { assert.equal(deservesCanvasCard({ mode: 'agent', status: 'stopped' }), true); }); + +test('a user-stopped workflow run keeps its card even though the executor stamped closed_at', () => { + // The bug the packaged drill found, and the reason this asserts the FIELD COMBINATION rather than + // trusting the rule's prose: the executor closes the session at the end of every step, including a + // stopped one, so `closed_at` is always set here. Gating on it made the fix dead code and the card + // vanished ~2s after Stop on the shipped build. + assert.equal( + deservesCanvasCard({ + workflow_run_id: 'run-1', + status: 'stopped', + ended_by_user: true, + closed_at: '2026-08-28T19:33:47.873352', + }), + true, + ); +}); + +test('a workflow run that ended ON ITS OWN still despawns, closed_at or not', () => { + // The nightly-litter case the original rule exists for. Losing this is how the fix becomes a leak. + for (const closed_at of [null, '2026-08-28T19:33:47.873352']) { + assert.equal( + deservesCanvasCard({ workflow_run_id: 'run-2', status: 'completed', ended_by_user: false, closed_at }), + false, + ); + } +}); diff --git a/frontend/src/shared/state/isUserLaunchedSession.ts b/frontend/src/shared/state/isUserLaunchedSession.ts index 0413483b..a64fe5da 100644 --- a/frontend/src/shared/state/isUserLaunchedSession.ts +++ b/frontend/src/shared/state/isUserLaunchedSession.ts @@ -10,6 +10,7 @@ export interface SessionOrigin { ended_by_user?: boolean; // Set by Close, never by Stop. It is what tells the two apart here. closed_at?: string | null; + dismissed_by_user?: boolean; } /** A run that is still working, so its card is worth having on screen. */ @@ -38,7 +39,17 @@ export function isUserLaunchedSession(session: SessionOrigin): boolean { * this could not tell them apart, so pressing Stop on a workflow-backed agent made its card vanish * along with the transcript the user stopped it to read (ENG-421). Someone stops a run to LOOK at * it; the nightly-litter case it was written for is the one that ends by itself, which still - * despawns. Close is what dismisses a card, and Close is the only route that stamps `closed_at`. + * despawns. + * + * `closed_at` is NOT the release, and assuming it was made this whole rule dead code on the only + * path it exists for. The workflow executor closes the agent session at the end of EVERY step, + * including one the user stopped, purely so the run sorts into chat history (`get_history` orders by + * `closed_at`, and nulls fall off the first page). So `closed_at` means "this run is over", not + * "the user dismissed this card", and gating on it deleted the card ~2s after Stop. Caught on the + * packaged build; dev never showed it because the drill there stopped a plain agent, not a + * workflow-backed one. Dismissal is the layout's job: closing the card removes it from the + * dashboard, which is where a user's "I'm done with this" already lives. The backend now says which + * of the two happened: `dismissed_by_user` is stamped by the close route alone. */ export function deservesCanvasCard( session: SessionOrigin & { status?: string }, @@ -46,5 +57,5 @@ export function deservesCanvasCard( if (isUserLaunchedSession(session)) return true; if (!session.workflow_run_id) return false; if (LIVE_STATUSES.has(session.status ?? '')) return true; - return !!session.ended_by_user && !session.closed_at; + return !!session.ended_by_user && !session.dismissed_by_user; }