Skip to content

fix(workspace): history never carries a turn's cost (#3063) - #3075

Merged
vybe merged 2 commits into
devfrom
fix/3063-portal-history-cost
Sep 29, 2026
Merged

vybe merged 2 commits into
devfrom
fix/3063-portal-history-cost

Conversation

@dolho

@dolho dolho commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • GET /api/enterprise/client-portal/agents/{name}/history sent cost on every message to every principal, including a portal token (an external client), against the Workspace contract. It is now projected out: PortalHistoryMessage no longer declares it, so the route's response_model strips it. The DB row is unchanged.
  • Dropped for every principal, platform included. Nothing reads it: the frontend's replyFromHistory copied it into a reply object whose cost no caller used, so that dead copy is removed as well. (AC 2: "otherwise drop them for everyone".)

Decision: execution ids stay (deviation from the AC's "no execution_id anywhere")

in_flight_execution_id and last_turn_outcome.execution_id are the client's own turn handles, not internal data about anyone else:

Removing them would break reattach and the failed-turn verdict. If the contract should forbid raw ids on the portal surface entirely, the fix is an opaque per-turn handle across dispatch, stream, cancel and history. That is a separate, larger change and can be a follow-up.

Changes

  • src/backend/client_portal/models.py: cost removed from PortalHistoryMessage.
  • src/frontend/src/components/portal/portalUtils.js: the dead cost copy is removed from replyFromHistory, and portalReplyRateable.spec.js is updated to match.
  • tests/unit/test_3063_portal_history_no_cost.py (new).

Test Plan

  • Red first: the new test drives the real route over a real sqlite row written with cost=0.4242, for a portal token and a platform principal, plus the reply poll's ?limit= read. On dev it fails with cost reached the wire … "cost": 0.4242; with the fix it passes. A positive control asserts the costed row is in the response.
  • 120 passed on seed 12345: the new file plus 2694 history window, ent534 voice, 2320 failed-turn visibility, 163 delegate scope, and models_centralized.
  • Frontend npm run test:unit: 180 files, 3723 tests. npm run build OK.

Fixes #3063

🤖 Generated with Claude Code

GET /client-portal/agents/{name}/history serialised `cost` on every message to
every principal, a portal token (an external Workspace client) included. The
Workspace contract excludes costs for external clients, and every other
projection already honours it. The history door was the one that leaked.

- PortalHistoryMessage no longer declares `cost`, so the route's response_model
  strips it. The row still stores it.
- Dropped for EVERY principal, not split by principal: nothing reads a history
  message's cost. replyFromHistory copied it into a reply object whose `cost`
  no caller used, so that dead copy goes too.
- Execution ids are kept on purpose. in_flight_execution_id and
  last_turn_outcome.execution_id are the client's OWN turn handles: the 202
  dispatch response issues the same id, and the client streams, cancels and
  reattaches by it. Removing them would break reattach-after-reload and the
  failed-turn verdict (#2320).

Test: test_3063 runs the real route against a real sqlite row written with a
cost of 0.4242, for a portal token and a platform principal, plus the reply
poll's narrow read. It was red on dev ("cost reached the wire … 0.4242") and is
green now. It has a positive control that the costed row is in the response.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dolho

dolho commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

/review — #3075

The history projection is right, but two other routes still send a turn's cost to a portal token. No critical findings; those two are informational.

Scope: clean. cost is removed from PortalHistoryMessage for every principal. Keeping the execution ids is a stated, reasoned deviation from the AC.

Execution coverage

Changed Executed by Live consumer Verdict
PortalHistoryMessage (no cost) test_3063_* through the real GET …/history route, real sqlite row with cost=0.4242, portal and platform principals, plus the ?limit= poll router.py:1550 response_model=PortalHistory ✅ executed
replyFromHistory (dead cost copy removed) portalReplyRateable.spec.js PortalConversation.vue reply poll ✅ executed

Fix mutation: on the merge-base, the new route test is red (cost reached the wire … "cost": 0.4242). With the fix it is green, and a positive control proves the costed row is in the response.

Checked and clean:

  • SQL, auth and credentials: model-only change, no new route.
  • Enum completeness: n/a.
  • Other readers of history cost: none in the backend, MCP or frontend.
  • Docs: nothing mentions history cost.
  • Merges cleanly with dev.

Informational (the same class, on sibling paths)

[I1] Sync chat still returns cost to portal tokens (confidence 9/10)

  • Where: client_portal/models.py:353 declares PortalChatResponse.cost: Optional[float], and service.py:3228 fills it with return {"response": reply, "cost": cost, ...}.
  • Who sees it: POST /agents/{agent_name}/chat (router.py:1176) serves every portal principal. It is the fallback path for streaming.
  • Suggested fix: nothing in the frontend reads .cost off this response, so the same one-line model fix as history applies, plus one route assertion.

[I2] The live stream relays the agent's raw result event, including total_cost_usd (confidence 8/10)

  • Where: router.py portal_stream_execution proxies agent log entries verbatim (async for chunk in response.aiter_text(): yield chunk).
  • What it carries: the agent server's stream_execution_log emits Claude Code's stream-json entries as-is (json.dumps(entry)), and the final {"type": "result", "total_cost_usd": …} entry is one of them (agent_server/services/stream_parser.py:140).
  • Suggested fix: for non-platform principals, drop cost and usage fields from result frames in the proxy. That needs SSE-frame-aware filtering, which is a bigger change than I1.

Both are the principal's own turns, so the severity is the same as the issue's (a contract breach, not a cross-tenant leak). Either widen this PR (I1 is trivial) or file a follow-up for I1 and I2 before Fixes #3063 closes the issue on the understanding that portal clients no longer receive costs.

@obasilakis

Copy link
Copy Markdown
Contributor

@dolho validated: the history fix is correct. Reverting the model change turns all 4 new tests red, and Tier 1 CI is green.

One change needed before merge:

  • Sibling leak: POST /client-portal/agents/{name}/chat still returns cost to portal tokens via PortalChatResponse.cost (client_portal/models.py:353, filled at service.py:3228). It is the synchronous fallback (clientPortal.js:700) and breaks the same Workspace rule. Please drop it there too, with a test on that route matching the history one.

Non-blocking: keeping in_flight_execution_id / last_turn_outcome.execution_id departs from #3063 AC 1/3 ("no execution_id anywhere"). The reattach/cancel rationale holds. @AndriiPasternak31, since those ACs are yours, can you confirm this is OK, or say if you want an opaque-handle follow-up?

@obasilakis obasilakis added the status-needs-fix PR has an unaddressed review/validation finding; cleared by the author's next push (#2815) label Sep 29, 2026
#3063)

POST /client-portal/agents/{name}/chat, the streaming fallback, still
declared PortalChatResponse.cost, so a portal token received the turn's
cost. Drop the field; the service keeps returning it for internal
accounting and response_model strips it. Route test for both principals
(positive control on response/message_id) plus a model-level pin.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dolho

dolho commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

@obasilakis, the sibling leak is fixed in fea7e36.

  • Fix: PortalChatResponse no longer declares cost. The service still returns it for internal accounting; the route's response_model drops it.
  • Route test: test_the_sync_chat_reply_carries_no_cost, parametrized over a portal token and a platform principal. It drives the real POST …/chat with a service result that carries cost, and checks as a positive control that response and message_id are on the wire.
  • Model-level pin: test_the_chat_response_model_declares_no_cost.
  • Mutation: with cost restored on the model, these 3 new tests fail; the other 4 still pass.
  • Related portal suites (bounded reply poll, ent79 exposure, 2320, 2580, ent287, ent403, 3012, 2694, models-centralized, seed 12345): 251 passed, 2 failed. The same 2 fail on the head without this commit (a _FakeDb missing get_setting_value and a MagicMock await), so they aren't caused by this change.

Still open: the live SSE stream relays Claude Code's final result frame, including total_cost_usd (I2 in the earlier /review). Fixing that needs frame-aware filtering in portal_stream_execution, so I'd do it as a follow-up rather than in this PR.

🤖 Generated with Claude Code

@github-actions github-actions Bot removed the status-needs-fix PR has an unaddressed review/validation finding; cleared by the author's next push (#2815) label Sep 29, 2026
@vybe vybe added the ui PR touches the frontend UI — triggers Playwright e2e tests label Sep 29, 2026

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge-train: batch validated on train/20260929-1926 (#3095)

@vybe
vybe merged commit 3a03919 into dev Sep 29, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ui PR touches the frontend UI — triggers Playwright e2e tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants