Skip to content

Commit ca104f0

Browse files
committed
feat(runner): every approval gate leaves a durable interaction row
1 parent 102aa5d commit ca104f0

4 files changed

Lines changed: 218 additions & 28 deletions

File tree

api/oss/tests/pytest/unit/sessions/test_interactions_dispatcher.py

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@
55

66
from unittest.mock import AsyncMock, MagicMock
77

8+
from oss.src.apis.fastapi.sessions.models import SessionInteractionCreateRequest
9+
from oss.src.core.sessions.interactions.dtos import SessionInteractionKind
10+
811
from oss.src.tasks.asyncio.sessions.interactions_dispatcher import (
912
InteractionsDispatcher,
1013
)
@@ -31,6 +34,19 @@ def _make_interaction(*, with_refs=True):
3134
)
3235

3336

37+
def test_create_request_accepts_omitted_workflow_references():
38+
request = SessionInteractionCreateRequest(
39+
session_id="sess-no-refs",
40+
turn_id="turn-no-refs",
41+
token="token-no-refs",
42+
kind=SessionInteractionKind.user_approval,
43+
data={"request": {"tool": "bash", "args": {"command": "pwd"}}},
44+
)
45+
46+
assert request.data is not None
47+
assert request.data.references is None
48+
49+
3450
async def test_respond_fallback_calls_invoke_when_no_dispatch_fn():
3551
interaction = _make_interaction()
3652
project_id = uuid4()
@@ -60,6 +76,35 @@ async def test_respond_fallback_calls_invoke_when_no_dispatch_fn():
6076
assert invoke_kwargs["user_id"] == user_id
6177

6278

79+
async def test_respond_without_references_builds_a_safe_reference_less_request():
80+
interaction = _make_interaction(with_refs=False)
81+
project_id = uuid4()
82+
user_id = uuid4()
83+
84+
interactions_service = MagicMock()
85+
interactions_service.fetch_interaction = AsyncMock(return_value=interaction)
86+
87+
workflows_service = MagicMock()
88+
workflows_service.invoke_workflow = AsyncMock(return_value=SimpleNamespace())
89+
90+
worker = InteractionsDispatcher(
91+
workflows_service=workflows_service,
92+
interactions_service=interactions_service,
93+
)
94+
95+
await worker.respond(
96+
project_id=project_id,
97+
user_id=user_id,
98+
interaction_id=interaction.id,
99+
answer={"approved": True},
100+
)
101+
102+
workflows_service.invoke_workflow.assert_awaited_once()
103+
invoke_request = workflows_service.invoke_workflow.await_args.kwargs["request"]
104+
assert invoke_request.references is None
105+
assert invoke_request.session_id == interaction.session_id
106+
107+
63108
async def test_respond_detached_calls_dispatch_fn_not_invoke():
64109
interaction = _make_interaction()
65110
project_id = uuid4()

docs/design/agent-workflows/projects/sessions-takeover/arda-handoff.md

Lines changed: 30 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -239,28 +239,33 @@ Out of scope for now: session deletion and live mid-turn attach.
239239
- PR #5382 and PR #5436 descriptions, for the exact shape of what merges beneath you. Your
240240
own PR #5426 description, for the record of what you built and where each piece landed.
241241

242-
## 6. Two questions Mahmoud wants your read on
243-
244-
Neither is settled. Your view as the frontend owner is why he is asking.
245-
246-
### 6.1 The streams-table rename
247-
248-
The table named `session_streams` stores no streamed frames; the architecture document opens
249-
by killing exactly that mental model (architecture document, page 1, mental model 2). It
250-
holds liveness and ownership, and it now also carries the session identity, the `name` and
251-
`description` header. The name misleads on both counts. The open question is whether to rename
252-
the table so its name matches what it holds. You read and write this row from the frontend
253-
(session fetch, and the rename UI you are about to wire in task 4), so the churn and the
254-
clarity both land partly on you. Weigh in on whether the rename is worth it and what the name
255-
should be.
256-
257-
### 6.2 The interactions guard ruling
258-
259-
The runner refuses to create an interaction row when the run context carries no
260-
`workflow_revision` reference, because the respond path could not re-invoke anything without
261-
it. The consequence is that gates in that state are answerable in-band only and are invisible
262-
to any interactions-plane inbox; a real QA session had approval records but zero interaction
263-
rows for exactly this reason (architecture document, section 5, "Sharp edges"). The open
264-
question is whether to keep that guard as is, accepting that some gates never get a durable
265-
interaction row, or to relax it. It matters to any future approval-inbox surface, which is
266-
your territory. Weigh in on which way it should go.
242+
## 6. Where the open questions landed, and the one Mahmoud wants your proposal on
243+
244+
Two questions this document originally left open were ruled by Mahmoud on 2026-07-21, so
245+
you inherit the answers rather than the debates.
246+
247+
**The streams table keeps its name.** `session_streams` stays as it is. Since the name
248+
misleads (the table stores no streamed frames; it holds liveness, ownership, and the
249+
session's name and description header), keep the architecture document nearby when working
250+
against it: the row is the session header plus status, and only the service read that
251+
overlays the live Redis state tells the truth about liveness.
252+
253+
**The interactions guard is relaxed: every gate gets a durable row.** Today the runner
254+
skips creating the interaction row when the run context carries no `workflow_revision`
255+
reference, which leaves those gates invisible to any future inbox and without an audit
256+
trail. The ruling: create the row anyway, with the reference left empty when absent. The
257+
runner-side change is queued on our side; what it means for you is that the future inbox
258+
must tolerate and sensibly display rows that cannot be attributed to an agent (group them
259+
under the session).
260+
261+
### The question that is yours: what should rejecting one card do to its siblings?
262+
263+
Today rejecting an approval card rejects only that card; other pending cards stay open.
264+
OpenCode ships the opposite policy: one rejection cancels every pending card in the
265+
session, on the reasoning that a rejection means "stop what you are doing" rather than "no
266+
to this one thing" (see the OpenCode comparison in this folder, the approval flow section).
267+
That policy would also have turned the db58551b incident's dead conversation into a clean
268+
stop. Mahmoud's ruling is that this is a product-feel call on your surface, and he wants
269+
your proposal: keep per-card rejection, adopt the cascade, or something between (for
270+
example, cascade only when the rejection carries no message). Bring the UX reasoning with
271+
the proposal.

services/runner/src/sessions/interactions.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,7 @@ type Reference = { id?: string; slug?: string; version?: string };
1818

1919
export type InteractionData = {
2020
request?: { tool: string; args: unknown };
21-
// The workflow references that identify which revision THIS turn is running, so the
22-
// respond invoke re-resolves the SAME workflow. We store the references (pointers), not
23-
// the revision data itself — respond resolves the live revision from them at invoke time.
21+
// Optional attribution for out-of-band re-invocation; inbox/audit rows exist without it.
2422
references?: Record<string, Reference>;
2523
};
2624

services/runner/tests/unit/session-keepalive-approval.test.ts

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1585,6 +1585,148 @@ describe("runTurn: real approval park + respondPermission resume", () => {
15851585
await env.destroy();
15861586
});
15871587

1588+
it("creates and resolves a durable gate row without workflow context", async () => {
1589+
const posted: Array<{ url: string; body: Record<string, unknown> }> = [];
1590+
const fetchSpy = vi
1591+
.spyOn(globalThis, "fetch")
1592+
.mockImplementation(async (input, init) => {
1593+
posted.push({
1594+
url: String(input),
1595+
body: JSON.parse(init?.body as string) as Record<string, unknown>,
1596+
});
1597+
return new Response(JSON.stringify({ ok: true }), { status: 200 });
1598+
});
1599+
1600+
try {
1601+
const { calls, deps, captured } = pausableHarness();
1602+
deps.hydrateHarnessSessionFromDurable = async () => {};
1603+
const noWorkflowRequest: AgentRunRequest = {
1604+
...engineReq,
1605+
...auth,
1606+
sessionId: "sess-no-workflow",
1607+
turnId: "turn-no-workflow-start",
1608+
};
1609+
const acquired = await acquireEnvironment(noWorkflowRequest, deps);
1610+
assert.equal(acquired.ok, true);
1611+
if (!acquired.ok) return;
1612+
const env = acquired.env;
1613+
1614+
const firstTurn = runTurn(
1615+
env,
1616+
noWorkflowRequest,
1617+
undefined,
1618+
undefined,
1619+
{ approvalParkMode: true },
1620+
);
1621+
await flush();
1622+
captured.onEvent!(
1623+
updateEvent({
1624+
sessionUpdate: "tool_call",
1625+
toolCallId: "tc-no-workflow",
1626+
title: "commit",
1627+
rawInput: { message: "hi" },
1628+
}),
1629+
);
1630+
captured.onPermissionRequest!({
1631+
id: "perm-no-workflow",
1632+
availableReplies: ["once", "reject"],
1633+
toolCall: {
1634+
toolCallId: "tc-no-workflow",
1635+
name: "commit",
1636+
rawInput: { message: "hi" },
1637+
},
1638+
});
1639+
const paused = await firstTurn;
1640+
assert.equal(paused.stopReason, "paused");
1641+
await flush();
1642+
1643+
const createCall = posted.find(({ url }) =>
1644+
url.endsWith("/sessions/interactions/"),
1645+
);
1646+
assert.ok(createCall);
1647+
assert.equal(createCall.body["session_id"], "sess-no-workflow");
1648+
assert.equal(createCall.body["turn_id"], "turn-no-workflow-start");
1649+
const createData = createCall.body["data"] as Record<string, unknown>;
1650+
assert.equal("references" in createData, false);
1651+
1652+
const parked = env.parkedApprovals.get("tc-no-workflow");
1653+
assert.ok(parked);
1654+
assert.ok(parked.promptPromise);
1655+
env.clearTurn();
1656+
const resumedTurn = runTurn(
1657+
env,
1658+
approveResume(true, {
1659+
sessionId: "sess-no-workflow",
1660+
turnId: "turn-no-workflow-resume",
1661+
}),
1662+
undefined,
1663+
undefined,
1664+
{
1665+
approvalParkMode: true,
1666+
resume: {
1667+
decisions: [
1668+
{
1669+
permissionId: parked.permissionId,
1670+
reply: "once",
1671+
toolCallId: parked.toolCallId,
1672+
toolName: parked.toolName,
1673+
args: parked.args,
1674+
interactionToken: parked.interactionToken,
1675+
promptPromise: parked.promptPromise,
1676+
},
1677+
],
1678+
carriedForward: [],
1679+
},
1680+
},
1681+
);
1682+
for (
1683+
let attempt = 0;
1684+
attempt < 10 &&
1685+
!posted.some(({ url }) =>
1686+
url.endsWith("/sessions/interactions/transition"),
1687+
);
1688+
attempt += 1
1689+
) {
1690+
await flush();
1691+
}
1692+
1693+
const resolveCall = posted.find(({ url }) =>
1694+
url.endsWith("/sessions/interactions/transition"),
1695+
);
1696+
assert.ok(
1697+
resolveCall,
1698+
JSON.stringify({ posted, permissionReplies: calls.permissionReplies }),
1699+
);
1700+
assert.deepEqual(resolveCall.body, {
1701+
session_id: "sess-no-workflow",
1702+
token: parked.interactionToken,
1703+
status: "resolved",
1704+
resolution: {
1705+
verdict: "approved",
1706+
tool_call_id: "tc-no-workflow",
1707+
},
1708+
});
1709+
1710+
captured.onEvent!(
1711+
updateEvent({
1712+
sessionUpdate: "tool_call_update",
1713+
toolCallId: "tc-no-workflow",
1714+
status: "completed",
1715+
content: "committed",
1716+
}),
1717+
);
1718+
calls.resolvePrompt!({
1719+
stopReason: "complete",
1720+
usage: { inputTokens: 1, outputTokens: 1 },
1721+
});
1722+
const resumed = await resumedTurn;
1723+
assert.equal(resumed.ok, true);
1724+
await env.destroy();
1725+
} finally {
1726+
fetchSpy.mockRestore();
1727+
}
1728+
});
1729+
15881730
it("preserves a denied call failed frame while a sibling gate is carried", async () => {
15891731
const { calls, deps, captured } = pausableHarness();
15901732
const acquired = await acquireEnvironment(engineReq, deps);

0 commit comments

Comments
 (0)