fix(reports): the five review findings on ent#365, which merged before the review landed

[dolho]

PR #2383 merged at 12:57; its re-review posted after. The findings therefore live in dev, so they are fixed here rather than on the branch.

The disclosure one

Client email addresses reached agent LLM context through MCP list_reports. addressed_to was appended to _SUMMARY_COLUMNS so an operator can answer "who was this produced for" — the support question a deliverable creates. But list_reports returns the backend's rows verbatim, and filterReportsForAgentScope filters rows by agent, never fields.

So an agent holding an agent_permissions edge to another agent could call list_reports({agent_name: "A"}) and pull A's clients' email addresses into its own context, where none were exposed before. It also contradicted the tool's own advertised contract — "Returns METADATA only (id, type, title, period, created_at)".

Stripped for every principal rather than only agent-scoped keys: a tool result is LLM context wherever it lands, and an operator still gets the field from the REST surface the UI uses.

"Can be addressed" had drifted from "can reach"

db.email_has_agent_access returns True for any user whose role is admin, regardless of sharing; the Workspace read gate is agent_on_roster — agent_sharing ∪ owned. A report addressed to a platform admin who neither owns nor is shared the agent stored happily and then 404'd in that admin's Workspace: a publish nobody could read, falsifying the invariant stated in the code comment directly above the check.

The publish now gates on the same function the reader uses. A test pins that, because two predicates that happen to agree today is exactly how this drifted.

In-chat cards never appeared for a synchronous turn

_resolve_portal_session resolves a report's chat from the ent#286 reverse marker — and mark_turn_inflight had exactly one caller, inside start_portal_turn. The POST .../chat path never set it, so a report published from a turn that arrived there stored portal_session_id = NULL and the card silently never rendered.

Not an edge case: /chat is ent#83's documented headless integration surface and the browser's fallback when the streaming dispatch route is unavailable.

The execution row is now pre-created on that path too — exactly what start_portal_turn already does, so the two converge instead of this one growing its own turn machinery — giving an id to mark with. Cleared in a finally: a marker that outlives its turn is worse than none, because the UI reattaches to a turn that ended and waits out the whole TTL.

The chat card fetched the whole payload

It called fetchAgentReport with no options, so rows_limit was omitted and the detail route returned the payload whole. A tabular deliverable near the 5 MiB REPORT_PAYLOAD_MAX_BYTES ceiling shipped all of it to the browser and rendered every row — inside a chat card — while the sibling Reports tab pages the identical report through #2162's window. There was no bounded read available here at all.

The card now takes a first page (50 rows) and says what it is a slice of. Deliberately not a "load more": the card is a preview inside a conversation, and the agent page is the reading surface — pointing there is more honest than paging a preview.

The create response's addressed_to was always null

Report extends ReportSummary, so the field is in model_fields — the filter was silently dropping it because db.create_report returns the column under its DB name addressed_to_email. The same model therefore meant two different things depending on the route: populated on GET /reports/{id}, always null on create. The MCP tool papered over it by echoing back the caller's own argument. Mapped explicitly.

Tests

test_ent365_report_audience.py gains: an admin who is not on the roster cannot be addressed; the publish gate is structurally the read gate; the create response reports the audience it stored. Existing stubs repointed from db.email_has_agent_access to agent_on_roster.

test_portal_stamps_the_surface_on_its_executions is generalised from a hard count of 2 to "one stamp per creation site", so adding the third is a decision rather than a broken test.

Full frontend suite green (58 files / 1244). Backend: 676 passed across the report/portal slice.

Related to Abilityai/trinity-enterprise#365

🤖 Generated with Claude Code

[dolho]

Code review — 6e75d95

Eight findings. The first does not compile, and three of the rest are in the new synchronous-marker code, which turns out to be the risky part of this change rather than the incidental one.

Critical

1. src/mcp-server/src/tools/reports.ts:49 — the TypeScript build fails.

stripAudienceFromReports<T extends Record<string, unknown>> cannot accept ReportSummary[]: ReportSummary is an interface (types.ts:432) with no index signature, and TypeScript gives interfaces no implicit one. Reproduced directly with the repo's own compiler:

src/tools/reports.ts(359,72): error TS2345: Argument of type 'ReportSummary[]' is not
assignable to parameter of type 'Record<string, unknown>[]'.
  Index signature for type 'string' is missing in type 'ReportSummary'.

npm run build in src/mcp-server will not compile. <T extends object>, or take and return ReportSummary[] directly.

High

2. src/mcp-server/src/tools/reports.ts:417 — the disclosure is only half-closed; get_report still leaks the address.

_mapping_to_report (db/reports.py:93) sets "addressed_to": row["addressed_to_email"], GET /api/reports/{id} returns it, and the get_report tool does a bare JSON.stringify(report). The exploit path this PR describes still works with one extra hop: a permitted sibling agent calls list_reports({agent_name:"A"}) — ids and agent_name are still returned, by design — then get_report(id), and A's client email lands in its LLM context. The same strip belongs in getReport.

3. client_portal/service.py:1500 — the sync path's marker clobbers a live streaming turn's, producing the #2120 double-spend.

mark_turn_inflight overwrites portal_inflight:{session} unconditionally. Streaming turn A is in flight (marker = A); a second send arrives on the same thread through POST .../chat — another tab, or the ent#83 headless surface. B pre-creates a row, overwrites the marker to B, and then run_resumable_turn raises ResumeLockBusy because A holds the resume lock. _run_sync_turn_and_clear_marker's finally calls clear_turn_inflight(session, B); current == B, so the session marker is deleted while A is still running and billed. The watching client is told nothing is in flight and is offered Retry — precisely what clear_turn_inflight's own compare-and-delete exists to prevent. That guard cannot help here, because the sync path is the writer that destroyed the value.

4. client_portal/service.py:1502 — the pre-created row is never terminated when the turn does not start.

create_task_execution inserts with status = RUNNING. ResumeLockBusy is raised inside run_resumable_turn's async with ResumeLock(...), i.e. before execute_task — so nothing writes a terminal for the row just created, and the helper's finally only clears the marker. start_portal_turn calls _fail_unstarted_execution for exactly this case; its docstring spells out the consequence ("leaves it RUNNING forever… the cleanup watchdog eventually fabricates a FAILED 'silent launch failure' against a perfectly healthy agent"). The new creator has no equivalent, so every 429 retry on a busy thread mints another orphan RUNNING row → canary E-01 critical plus fabricated FAILED rows. Same hole for any unexpected raise out of run_resumable_turn.

Medium

5. client_portal/service.py:1504 — marker TTL is turn_timeout + 60 instead of portal_max_turn_seconds(turn_timeout).

That helper (2 * (turn_timeout + 10 + _AUTO_RETRY_MAX_TIMEOUT_S) + 60) exists because run_resumable_turn can run the whole turn twice on a cold retry, and its docstring says sizing the marker at a single timeout "reintroduces exactly what #2120 fixed". At the default 3600s the streaming path gets ~7200s and this gets 3660s. So a resume whose JSONL is gone burns attempt 1, retries cold, and the marker expires mid-attempt-2 — a report published at the end of that turn resolves portal_session_id = NULL and the card never renders. That is the bug this PR is fixing, reintroduced by its own fix on the retry path.

6. routers/reports.py:207 — include_owned=False re-opens the same drift in the opposite direction.

The read gate is _require_roster(agent, principal.email, principal.is_platform) → include_owned=True for a platform session, and get_report_for_client matches on addressed_to_email. So an owner reading their own agent in the Workspace (the ent#357 one-click flow) can read a report addressed to them, while publishing one is now refused with 400 — this PR's own "can reach but cannot be addressed" defect, mirrored. And the remediation text is impossible to follow: "share the agent with that address first", when Trinity refuses a self-share. Pass the caller's is_platform equivalent, or accept the owner explicitly.

Low

7. client_portal/service.py:1246 — the sync row stores a different message than the streaming row, contradicting the helper's own docstring. _precreate_sync_execution(agent_name, message, email) is called after message = (manifest_prefix + message) if resuming else cold_message, so on a cold turn schedule_executions.message holds history_prefix + manifest + message — up to 20 prior turns. start_portal_turn creates its row with the client's raw message, and execute_task never rewrites message for a supplied execution_id. So the two paths are distinguishable (the docstring claims "indistinguishable rows"), and every cold sync turn duplicates the client's conversation history into an operator-visible row with 90-day retention. Pass the raw client message.

8. client_portal/service.py:1302 — stale docstring. portal_chat's #2214 note still says the synchronous path "passes nothing (it sets no marker)". It sets one now — and that sentence is the reason a future reader would not go looking for findings 3 or 5.

🤖 Generated with Claude Code

[AndriiPasternak31]

Review sweep — /review + /validate-pr (agent-assisted, findings only)

/review: needs-changes · /validate-pr: request-changes. All five re-review findings are genuinely fixed with tests — verified per finding, including the commit-2 get_report strip and the tsc-interface regression it pins. Three things before merge:

N1 — new defect: orphaned RUNNING row on the sync path's pre-dispatch refusal (src/backend/client_portal/service.py:1501-1522). _precreate_sync_execution writes a status=RUNNING, claude_session_id=NULL row; run_resumable_turn raises ResumeLockBusy at lock contention before dispatch (session_turn_service.py:333, mapped to 429 at service.py:1523) — the finally clears the marker but nothing terminal-writes the row. The streaming path closes this exact window via _fail_unstarted_execution in its except handlers (service.py:2140/2150); the sync path's caller is the HTTP router with no equivalent. Every contention hit (double-send on a busy thread through the sync route — the browser's documented streaming fallback, or an ent#83 headless client): 429 to the caller + a phantom RUNNING/NULL-session row → canary E-05 fires at >60s (major) → the 5-min no-session sweep FAILs it → false FAILED rows in the client's history and the fleet failures tile. Pre-PR, contention created no row at all. Fix: when owns_marker is true and the turn raised pre-dispatch (ResumeLockBusy at minimum), call _fail_unstarted_execution — the compensator the streaming path already uses.

N2 — the sync marker skips the #2320 stale-verdict clear: start_portal_turn runs clear_turn_outcome(session_id) immediately before mark_turn_inflight (service.py:2123-2124); the new sync-path marker (:1504) doesn't. Now that sync turns set a marker, a client reloading mid-turn enters the reattach flow and can be handed the previous turn's recorded failure verdict as this turn's — the exact #2320 hazard, re-opened narrowly on the fallback path. One line to mirror.

The docs now contradict the fix they document: docs/memory/requirements/core-agent.md §5.14 FR-1 still states the create route gates on db.email_has_agent_access — "the same predicate the #848 inline-auth path gates on, so 'can be addressed' cannot drift from 'can reach'" — which is precisely the sentence this PR's finding #2 proved false; it also omits the new owner-cannot-be-addressed rule. docs/memory/feature-flows/workspace-deliverables.md names the old predicate in the flow diagram (line 40) and prose (55-57), and lists /ws as the only deliberate addressed_to exclusion (107-111) while this PR adds a second (the MCP strip, every principal). Stale docstrings too: service.py:1738-1740 ("start_portal_turn — the only production caller") and :1299-1302 ("it sets no marker").

Test suggestions (non-blocking): finding #4 ships with no frontend test — nothing pins rowsLimit: 50, the "Showing the first X of Y" notice, or the rowMeta clear on thread switch; and the get_report strip call-site is unpinned (only the helper is tested — notable because commit 2 exists precisely because that call-site was missed once already).

[obasilakis]

Reviewed. No blocking findings, and I am deliberately leaving this as a comment rather than an approval because one item below is a scope decision rather than a defect — it needs an answer, not a patch.

Five review findings on a PR that merged before its review landed, fixed on dev rather than on a dead branch: that is the right call and the right place.

Verified fixed

The disclosure. stripAudienceFromReports is applied to both list_reports and get_report, which is the important part — stripping only the list would have left the same address one hop further on, since _mapping_to_report sets addressed_to and the detail route returns it. Applying it to every principal rather than only agent-scoped keys is the right reading: a tool result is LLM context wherever it lands. The T extends object signature with the regression test explaining why Record<string, unknown> broke tsc on an interface is a good test — it encodes the failure in the type system rather than in a comment.

The publish/read gate divergence. agent_on_roster(name, audience, include_owned=False) replacing db.email_has_agent_access is correct, and test_the_publish_gate_is_the_read_gate pinning it structurally is the part that keeps it correct. The comment is right that two predicates that happen to agree today is exactly how this drifted. include_owned=False is the right choice and the reasoning for it is stated.

The synchronous-turn marker. I checked the concern that the pre-created row might be orphaned — it is not: execution_id=execution_id is passed through to run_resumable_turn at client_portal/service.py:1544, so the row is adopted rather than duplicated, and the two paths genuinely converge on start_portal_turn's shape. _run_sync_turn_and_clear_marker's finally covers the raising exits including ResumeLockBusy, which is raised from inside run_resumable_turn and therefore inside the wrapper. owns_marker correctly means only the caller that set it takes it down.

The card's unbounded fetch. 50 rows plus row_meta.total, and saying what the slice is of rather than offering a "load more" inside a chat card, is the right shape — the agent page is the reading surface.

The always-null addressed_to on create. The diagnosis is the interesting part: the field was in model_fields, and the filter was dropping it on the DB column name. Mapping it explicitly and pinning it with a test is right.

Needs a decision, not a fix

The audience strip is at the MCP layer only.

stripAudienceFromReports lives in src/mcp-server/src/tools/reports.ts. An agent-scoped MCP key is a valid bearer token against the backend directly — that is how the heartbeat, the #1083 result callback and the reports write path all authenticate. So an agent can reach GET /api/reports/{id} and GET /api/reports over plain HTTP and read addressed_to for every agent its owner can access, which is a strictly wider set than the {self} ∪ permitted scope the MCP layer enforces.

The PR's own stated rationale is "a tool result is LLM context wherever it lands". That argument applies identically to a curl result in the agent's own shell — arguably more so, since the threat model here is a prompt-injected agent and such an agent has Bash.

I do not think this blocks: it is a residual on the same surface rather than a regression introduced here, and the operator genuinely needs the field on REST for the UI. But it should be resolved one way or the other rather than left implicit:

• strip for agent principals at the REST read (the field stays for JWT callers, which is where the UI reads it), or
• record the residual explicitly in the tool's docstring and in the flow doc, so the next reader does not conclude from the MCP strip that the address is unreachable by an agent.

Happy either way. What I would not want is the current state, where the code reads as though the hole is closed.

Minor

1. _precreate_sync_execution docstring — "run_resumable_turn then creates the row itself, exactly as before" reads as though the pre-created row goes unused. It only describes the fail-soft None path. Worth one clause, because I spent a while confirming this was not creating an orphan running row per sync portal turn.

2. PortalDeliverables.vue:shownRows — if the server omits limit, meta.limit ?? 0 makes shownRows zero while hasMoreRows stays true, and the card renders "Showing the first 0 of N rows". Cheap guard.

3. test_portal_stamps_the_surface_on_its_executions — generalising from a hard count of 2 to "one stamp per creation site" is a real improvement over the version it replaces. Noting only that source.count(...) == creates + 1 still couples the assertion to the number of non-creation dispatch stamps, so a second dispatch-site stamp would break it for a reason unrelated to what it tests.

Verdict

Content is good and the diagnoses are precise. Approving as soon as the MCP-versus-REST question above has an answer — either fix or a written residual is fine by me.

[dolho]

The MCP-vs-REST question, answered by closing it — 475b17f0

You offered a fix or a written residual. I took the fix, for the reason you gave yourself: "what I would not want is the current state, where the code reads as though the hole is closed." A residual would have left exactly that, one comment further away.

Your argument carries. An agent-scoped MCP key is a valid bearer token against the API directly — the heartbeat, the #1083 result callback and the reports write path all authenticate that way — so GET /api/reports and GET /api/reports/{id} returned addressed_to for every agent the owner can access, wider than the {self} ∪ permitted the MCP layer enforces. And "a tool result is LLM context wherever it lands" applies at least as strongly to a curl result, since the threat model is a prompt-injected agent and such an agent has Bash.

_hide_audience withholds the field from any caller that is not an interactive human, on all three read routes. The UI is untouched — it reads over a JWT, which is the allowlisted case.

Two details worth your eye:

• The predicate is shared, not copied. is_interactive_principal is extracted out of reject_non_interactive_principal, so the redact and the refuse have one definition. Not every use of that rule is a refusal, and two copies of it would have been the same drift this PR already fixed once for the publish/read gate.
• It stays an allowlist. mcp_api_keys.scope is free text with no CHECK constraint, so naming agent and connector would silently admit the next scope that ships (feat: machine identities for admin/ops APIs — service credentials that survive enforced 2FA #2323). A principal with no mcp_scope attribute fails closed via a sentinel rather than getattr(..., None) — that default would make an absent attribute the privileged value. Both directions are parametrized in the tests, including a hypothetical a_scope_from_2027.

The three minors

1. _precreate_sync_execution docstring. Fixed, and it now says which path is which: the "creates the row itself" sentence described only the fail-soft None path, and on the success path the id is threaded through and the row is adopted. Written explicitly because you spent time confirming it was not an orphan running row per sync turn — that cost is the signal that the sentence was wrong.

2. shownRows. Fixed. An absent limit now means "the slice was not bounded", so the shown count is everything after the offset. "Showing the first 0 of N rows" while hasMoreRows stayed true was a sentence that cannot be right.

3. The stamp count. You were right that creates + 1 couples to something unrelated. It now walks the AST and asserts the actual rule — every create_task_execution site in the module carries a source_channel stamp — so a second dispatch-site stamp elsewhere cannot break it, and an unstamped creation site is named by line number.

Verification

8 new tests, mutation-checked rather than asserted: disabling the strip turns 8 red. 394 pass across the reports / ent#365 / dependencies / auth-wiring suites; full frontend suite green.

Ready for re-review.

[obasilakis]

/validate-pr — approving. CI is fully green as of this review.

Fixing these on a branch off dev rather than on the original branch is the right call given #2383 merged at 12:57 and the re-review posted after — the findings live in dev, so this is where they belong.

The disclosure one is the important one, and it is closed in the right place

Stripping addressed_to in the MCP tool only would have left the hole open, because an agent-scoped MCP key is a valid bearer token against the REST API directly — that is how the heartbeat, the #1083 result callback and the reports write path all authenticate. An agent could have curled GET /api/reports and read client email addresses for every agent its owner can access, which is strictly wider than the {self} ∪ permitted scope the MCP layer enforces. The PR's own rationale for stripping at the tool — a tool result is LLM context wherever it lands — applies at least as strongly to a shell result, since the threat model is a prompt-injected agent and such an agent has Bash. Closing it at the REST read rather than recording a residual is right; the alternative leaves code that reads as though the hole is shut.

is_interactive_principal extracted from reject_non_interactive_principal so the two cannot drift is the correct shape — not every use of a predicate is a refusal, and this surface needs to redact a field rather than reject the request. The allowlist reasoning holds: mcp_api_keys.scope is free text with no CHECK constraint, so a denylist naming agent and connector is open at the top and silently admits the next scope that ships. The getattr(current_user, "mcp_scope", "__missing__") is None sentinel avoiding the #2323 trap — where an absent attribute becomes the privileged value — is the detail that makes it fail closed rather than nearly closed.

"Can be addressed" drifting from "can reach"

db.email_has_agent_access returns True for any admin regardless of sharing, while the Workspace read gate is agent_on_roster (agent_sharing ∪ owned). So a report addressed to a platform admin who neither owns nor is shared the agent stored happily and then 404'd in that admin's Workspace — a publish nobody could read, falsifying the invariant stated in the comment directly above the check. Gating the publish on the same function the reader uses, and pinning it with a test, is the fix; two predicates that happen to agree today is exactly how this drifted in the first place.

include_owned=False is the right argument, and the reason is worth having in the comment: an owner reads their agent's reports on the operator surface, so addressing one to themselves is not what the audience column is for.

The in-chat card never appearing for a synchronous turn

mark_turn_inflight having exactly one caller inside start_portal_turn meant /chat — ent#83's documented headless integration surface and the browser's fallback when streaming dispatch is unavailable — stored portal_session_id = NULL and the card silently never rendered. Both are live paths, so this was not an edge case.

Pre-creating the execution row on that path so there is an id to mark with, rather than growing separate turn machinery, converges the two paths instead of forking them. The finally clear matters for the stated reason: a marker that outlives its turn is worse than none, because the UI reattaches to a turn that ended and waits out the whole TTL. And the docstring now says explicitly that run_resumable_turn adopts the pre-created row rather than creating a second one — the earlier wording read as though the row went unused, which is a question that should not cost a reader ten minutes.

The response-shape contradiction

Good catch that the model_fields filter stopped meaning what its comment claimed the moment ReportSummary gained addressed_to. Report extends it, so the field is in model_fields, and the filter was silently dropping it only because db.create_report returns the column under its DB name. The same model meant two different things depending on the route — populated on GET /reports/{id}, always null on create — with the MCP tool papering over it by echoing back the caller's own argument. Mapping it explicitly so the response says what the row says is right.

The card fetching the whole payload

fetchAgentReport with no options omitted rows_limit, so a tabular deliverable near the 5 MiB ceiling shipped whole to the browser and rendered every row inside a chat card, while the sibling Reports tab paged the identical report through #2162's window. A first page is the right default for a preview inside a conversation. The shownRows fix is the subtler one — meta.limit ?? 0 rendered "Showing the first 0 of N rows" while hasMoreRows stayed true, which is a sentence that cannot be right; treating an absent limit as "everything after the offset" is correct.

The TypeScript regression test is a nice touch: constraining on Record<string, unknown> broke tsc because an interface has no implicit index signature, and expressing that regression in the type system is better than a comment asking someone not to do it again.

---

Checklist: base dev, security scan clean, dependencies.py and the other touched top-level modules are covered by the Dockerfile's COPY src/backend/*.py wildcard, no new os.getenv() vars, no schema change. All three surfaces stay in sync (backend router, MCP tool, frontend). CI green across all seeds, e2e and prod-image-smoke.

Two process notes, neither blocking:

No labels on this PR. Every sibling in this batch carries priority / type / theme. Given this is a P1-adjacent disclosure fix, priority-p1 + type-bug and a theme would be worth adding.

No issue reference with a closing keyword. This is a follow-up to a merged PR rather than to an open issue, so there is nothing that would auto-close correctly — but ent#365 is still status-in-progress and will need its status set by hand, since the cross-tracker automation is same-repo only.

Architecture doc. architecture.md describes the reports MCP tool's metadata contract; the audience redaction now also applies at the REST read for any key-authenticated caller. One line under Agent Reports would keep the doc honest about who sees addressed_to.