fix(workspace): the sync portal turn never carried its session, so report-back could not fire (#2426)

[dolho]

Journey Impact: none: repairs an existing surface's report-back on one code path — no user-facing promise is added or extended.

Closes #2426

The bug

ent#457 gave the Workspace a report-back: an agent that delegates during a chat turn gets the completion posted into that thread. It cannot fire on the synchronous chat path — the parent execution never receives the session binding, so report_completion drops every report at its own if not source_channel_chat_id gate.

Measured on a dev instance, split cleanly by path:

portal rows total: 8
missing chat_id : 5

07:55 → 09:04   chat=7d27744d…   browser, streaming path
09:06 → 09:09   chat=NULL        POST .../chat, synchronous path

Two correct changes that collide

ent#457 passes the binding down, and execute_task persists it — but only when it creates the row:

# task_execution_service.py:1095
if not execution_id:
    execution = db.create_task_execution(..., source_channel_chat_id=..., source_channel_client=...)

ent#365's _precreate_sync_execution has already created the row and handed the id over, so execute_task adopts it and skips that branch. And the pre-create stamped only the surface:

core_db.create_task_execution(..., source_channel=PORTAL_SOURCE_CHANNEL)   # no chat_id, no client

Its own docstring named the invariant it broke:

Mirrors start_portal_turn's creation exactly … so the two paths produce indistinguishable rows and a report published from either can be joined back to its chat.

And start_portal_turn's sibling comment says "both creation sites or the stamp is a coin flip depending on which path made the row." ent#457 covered the two sites that existed when it was written; ent#365 had added a third.

The fix

Stamp both fields in the pre-create. session_id is a required parameter, not optional — the value is in scope at the only call site, and a default would let a future caller silently reintroduce the inert row.

Rejected: teaching execute_task to UPDATE an adopted row. That widens a hot path used by every trigger to repair one caller's omission; the adopted row is the caller's to get right.

Two guards that were red on dev

backend-unit-test is failing on dev right now. Both failures are in this feature area and both are guards that had gone inert, so they're repaired here rather than left for the next PR to trip over. Frontend-only PRs stay green because the changes job path-filters the backend suite away — which is why nobody saw it.

test_both_portal_row_creation_sites_name_the_chat asserted a literal census, == 2 sites. It went red the moment the third site appeared — the guard working — and the bug it names shipped anyway. It now asserts the rule instead of the count:

assert destination == surface, "…a row created by the unstamped one can never be joined back to its chat"

test_portal_turn_kwargs_bind_against_execute_task parsed portal_chat for a literal run_resumable_turn(...) call. That call had moved into _run_sync_turn_and_clear_marker, where it is run_resumable_turn(**kwargs) — a splat, which names nothing — so the walk found no keywords and the guard asserted itself dead. It now reads the keywords where they're actually named (the wrapper's call site), scans both entry names, and subtracts the wrapper's own consumed parameters.

Neither rewrite loses coverage; both now fail for the reason their docstring gives rather than because a number or a call site moved. Say so if you disagree — I edited guards I didn't write.

Why the bug survived its own tests

Both features have passing tests:

• ent#457's mock the engine and assert the kwargs are passed — they are.
• ent#365's assert no orphan running row — still true.

Nothing asserted the persisted row, the only place the two meet. That is verbatim the lesson test_ent457_portal_turn_kwargs.py states about itself: "the only place the two signatures meet is here." The new suite asserts at that layer and adds a derived parity check, so a fourth channel field added to one writer and forgotten in another fails here rather than shipping as another silently-inert report path.

Verification

pytest -k "portal or ent457 or ent365 or 2426"
  before this branch:   2 failed, 400 passed     ← both pre-existing on dev
  after:              402 passed, 1 skipped

Mutation-checked:

Mutation	Result
Remove the new stamp	4 failed (new suite + the repaired census guard)
Feed execute_task an unknown kwarg	repaired binding guard fires

Impact

Client-facing effect is a missing notification, not a wrong one: a delegated job finishes and nobody is told — precisely the scenario ent#457 was written for. No schema change, no migration, no API change.

[obasilakis]

Approving. No code blockers on any axis, and I confirmed the headline claim empirically rather than by reading.

Verified

Exported the trees read-only via git archive and ran the suites:

tree	selection	result
PR base 8d9af18b	the two files	2 failed, 27 passed
PR base 8d9af18b	-k "portal or ent457 or ent365"	2 failed, 395 passed
origin/dev 8df57509	same + 2426	2 failed, 395 passed
base + this diff	same	402 passed

The failures are exactly assert 3 == 2 and "portal_chat no longer calls run_resumable_turn by that name / assert set()". dev is red right now, and this PR is what makes it green — the claimed 402 matches exactly.

Non-vacuity checks out too: applying only the test half of the diff to the unfixed base leaves the census test and all four new test_2426 tests red, so it is green for the right reason. The wrapper-parameter subtraction is not over-broad — wrapper ∩ run_resumable_turn and wrapper ∩ execute_task are both empty, the masked set is empty, and passed - rrt and passed - (rrt | wrapper) are identical. Post-change, forwarded is 8 kwargs including source_channel_client, i.e. the exact ent#457 regression, all accepted by execute_task. Neither rewritten guard can silently pass over a genuine defect.

Comments (non-blocking)

• The census test's premise is wrong, not merely loosely worded. surface = 3 counts service.py:1523/1548 — portal_chat's kwarg forwarded to _run_sync_turn_and_clear_marker — which is not a row-creation site, while the actual third creation site (task_execution_service.py:1103) lives in another file and is never counted. So test_both_portal_row_creation_sites_name_the_chat counts two creation sites plus one forwarding site. The pairwise equality it enforces is a real property; the name and comment assert something the test does not check.
• test_portal_turn_kwargs_bind_against_execute_task was dead, not wrong — it passes at base with only the test rewrite applied, so its repair is a resurrection rather than a second live defect fixed. The body's "two guards that were red" is accurate but shouldn't be read as two bugs.
• == 2 → >= 2 trades away a tripwire. The original census loudly announced "a new creation site appeared, go audit it", which is exactly what it did when ent#365 landed — nobody acted, and the bug shipped. The replacement enforces the pairing invariant, which is net stronger, but the "count moved" signal is gone.
• The string-literal census stays brittle: a future site stamping from a variable not named session_id goes red spuriously. Fail-loud direction, so acceptable.

Note for sequencing: #2430 edits the same client_portal/service.py regions (_precreate_sync_execution, start_portal_turn). Land this one first.