feat(workspace): addressed asks end-to-end in OSS — SQL narrowing, the agent-page leak, and the module itself (ent#428) - #2356
Conversation
6a2de44 to
1cc5a7a
Compare
…nt#428) `operator_queue.addressed_to_email` shipped in #2300, but nothing could ask the DB for "the asks addressed to this person" — the only consumer read a fleet-wide page of pending items and filtered the addressee out of the result. That is wrong at the layer, not just inefficient. `list_items` orders by status, then priority, then age, and applies `limit` before the caller ever sees a row, so a post-hoc filter really means "the newest N pending items in the FLEET, of which some happen to be yours". One person's low-priority ask falls out of the window as soon as the fleet is busy — it disappears from the surface that addressed it while still sitting pending in the queue, and by construction nobody else is permitted to answer it. So `list_items` takes an optional `addressed_to_email`, compared case-insensitively: the ingestion boundary lowercases before it stores (`_validated_addressee`), but `create_item` is a public writer and the read must not silently depend on every future caller having remembered that. Also annotates `respond_to_item(responded_by_id=...)` as Optional. It is a `users` id, and an ask answered by someone with no platform account has no row there — NULL id plus the answering email is what distinguishes that from an answer by an operator whose account was since deleted, which keeps its id. The annotation records the intent so it is not later "tidied" back to `str`. Additive: no migration, no route change, no default behaviour moved — omitting the argument returns exactly what it returned before, pinned by a test. Related to ent#428. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ewer's (ent#428)
Found by /review on the primitive commit, and reproduced before it was believed:
two clients co-shared on one agent, an ask addressed to the first, and the
second reads its title and question verbatim.
BOB SEES: [('Invoice Acme at the agreed discount?',
'Alice, confirm the Q3 discount before I send it.')]
`client_portal/agent_page.py::_asks` is the client-facing "Waiting on you"
block, and it was scoped by `agent_name` alone. One agent is routinely shared
with several people, so that scope was never the audience — it was the only
scope available before ent#364 added the addressee. The file already withheld
`context` as a known leak surface, which is what makes it deceptive: it looks
like a reader that has thought about disclosure. `title`, `question` and
`options` are agent-authored free text and are exactly where an ask meant for
someone else says something not meant for this reader. The section heading was
"Waiting on you" the whole time.
Pre-existing on `dev` since ent#364: that change added the column, its
ingestion-time validation and three new renderings that honour it, and did not
teach this older reader. Fixed here rather than filed, because this is the PR
that adds the primitive it needs.
`viewer_email` is REQUIRED, not defaulted — a default is how the next caller
silently gets the unfiltered list back.
Unaddressed operator asks stop rendering here, and that narrowing is intended:
this is a client-facing page, an operator ask is agent-authored text written for
the operator, and a client cannot act on one from here anyway ("reply in chat"
is the only affordance). Operators keep the full queue in Operations, and the
page now agrees with what the Workspace's other ask surfaces show that person.
Also makes the filter fail CLOSED. `list_items` uses truthiness for every other
filter, so a falsy value means "don't filter"; for this argument that would turn
a caller which lost its email into a caller that sees everyone's asks. `None`
still means no filter (the operator listing); `""` now matches nothing.
The two `test_ent360` failures were signature-only — both stub the query out, so
their subjects (alert-type exclusion, `context` never forwarded) are untouched;
they just pass the viewer now.
Three new tests, all three failing without this change. Slice re-run:
655 passed, 1 skipped.
Related to ent#428.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Everything about the Workspace lives in OSS. This surface was built as the entitled `workspace_asks` module in the private repo, and that was the wrong edition for exactly the reason ent#356 (the Workspace itself) and ent#443 (multi-agent rooms) were: the frontend that drives it — `PortalAsks.vue`, `store.fetchAsks()`, `store.asksAvailable` — ships in EVERY build and self-disables on a 404. So a community install rendered the ask affordance and then refused it: an advert for a missing feature rather than a clean absence. Under `client_portal/`, not a new top-level package. That is what it is — a client-portal surface, on the client-portal prefix, owning no table (an ask IS an `operator_queue` row with an addressee, which is what makes "answering anywhere clears it everywhere" true by construction). It also means the module inherits `client_portal`'s Dockerfile COPY line instead of needing its own: a new top-level package that `main.py` imports at module scope builds a clean image that dies at import, and that has now been #1033, ent#356 and ent#443. Verbatim port. The service and models are unchanged; the router loses its `requires_entitlement` dependency and the service its `FEATURE_ID`. Nothing else moved, so the diff reads as a move. RETAINED, deliberately: the `/api/enterprise/client-portal/asks` prefix. ent#83 published it as the headless integration surface and the shipped Vue bundle already calls it — renaming breaks a client for nothing. Provenance, not a licensing claim, exactly like the `enterprise_`-prefixed portal tables it reads beside. Transition: the OSS router is included BEFORE `register_enterprise(app)`, so on an install whose submodule still registers the gated module both mount and the ungated OSS one wins the match order. Verified live on a pure-OSS build with an un-bumped submodule — OSS route first, `entitled features: []`. The suite moves with it (`tests/unit/test_ent428_workspace_asks.py`), unchanged beyond import paths and one path calculation, plus three cases the ported ones structurally cannot see: the routes answer with no entitlement, nothing calls `requires_entitlement`/`register_module` (parsed, not grepped — a docstring may still SAY it used to be gated), and the mount precedes the seam. That last assertion is anchored on the CALL, not on `str.index` of the name: `main.py` mentions `register_enterprise(app)` in prose above it, so a substring match finds the comment. It false-failed here on the first run. #2355 carries the same weaker form — it can only false-fail there, never false-pass, and is worth tightening separately. Local: 22 in the ported suite; `-k "portal or agent_page or ent364 or ent428 or operator or queue or asks or entitle or version"` → 808 passed, 1 skipped. Related to ent#428. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ims, and an N+1 (ent#428)
Four findings from /review on the move commit. None in the ported logic; all
four are things the move itself created or exposed.
**Three now-false edition claims.** Moving the module made public prose lie:
* `docs/memory/architecture.md:1241` said "the read/answer surface is the
ENTITLED `workspace_asks` module". A public doc calling a public module paid
is the wrong direction of wrong — it is the sentence a reader would trust.
* `stores/clientPortal.js` explained its own 404/403 guard as "the module is
enterprise-gated, so an OSS or unentitled build answers 404/403". The guard
still belongs — an OLDER backend either predates the surface or still gates
it — but the stated reason would tell a reader asks are a paid feature.
Comments only; the logic is untouched and still correct.
* `asks/models.py` still called itself `Private.`
This does mean the PR now touches one frontend file, so the "no frontend" claim
in the body is corrected rather than quietly left standing.
**An N+1 the move promoted to an OSS polled endpoint.** `list_asks` called
`_on_roster` per ITEM. That resolves to `agent_name in roster_agent_names(...)`,
one-to-two DB reads, for an answer that cannot change inside a single request —
on an endpoint the Workspace polls every 20s, per signed-in client, per open
tab. A client's asks cluster on one or two agents, so this is now 1-2 reads
instead of N.
The FIRST attempt at that fix was wrong and the tests said so: hoisting
`roster_agent_names` out of the loop turned six of them red, because the suite's
seam is `agent_on_roster`. That was correct feedback rather than an awkward
fixture — `agent_on_roster` IS the access predicate ("the scope of what a caller
can DO must equal the scope of what they can SEE"), and re-implementing
membership beside it is exactly how the two drift apart. Redone as a per-REQUEST
memo over that same predicate: same win, seam intact, fail-closed preserved per
agent.
Pinned on the CALL COUNT, not on timing — a duration assertion would be flaky
and would not say what it means. Proven non-vacuous:
AssertionError: roster consulted 8 times for 8 asks across 2 agents
Slice: 678 passed, 1 skipped. Guard sweep (`centralized|parity|invariant|
architecture|wiring|1310|186|293|926|enumeration`): 816 passed, 3 skipped.
Related to ent#428.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e surface that never rendered (ent#429) Slices 3+4 of ent#364. Most of the rendering shipped in #2300; auditing the acceptance criteria against the code left three things genuinely undone, one of them a promise the feature has been making falsely since it landed. **AC #2 — nothing homeless.** `workspace_session_id` was READ by the ask projection and written by nothing, so `chat_id` was always null and an ask raised by a scheduled run belonged nowhere. It is now resolved at RAISE time (`client_portal.service.ensure_thread_for_ask`), reusing the client's existing thread with that agent and opening one only if they have never chatted — the same `_resolve_session_id(..., None)` a first client turn takes, so an ask lands IN the conversation rather than beside it. Render-time resolution would not be an attachment: it is a guess repeated per view, with nothing durable to audit. **And it is platform-written, which nothing enforced.** The projection's docstring already promised "platform-written context only" while the clamp happily passed an agent-authored `workspace_session_id` straight through — `chat_id` is where the Workspace SENDS a reader, so an agent could choose that destination. Stripped unconditionally now, before the real one is written, and stripped whether or not an addressee resolves. Three ordering details, each load-bearing: * the addressee is resolved BEFORE the context block, because the thread id is written INTO context and doing it after would add bytes the size cap had already signed off on; * the context dict is REBUILT, not popped — `out = dict(req)` is a shallow copy, so popping would reach into the caller's own dict and the clamp's contract is that it never mutates the request; * the thread id survives the oversize-context marker, or an agent-controlled oversize context would be the one way to produce a homeless ask. Fail-SOFT, deliberately the opposite direction to `_validated_addressee` beside it: that fails closed because it is an authorization decision, this one only decides where a link points, and losing the whole question over a missing link is the worse trade. **AC #1 — the inline-in-chat surface has never rendered.** `PortalConversation` passes `props.agent`, the agent OBJECT, to `asksForAgent`, which compares it against the string `agent_name`. It matched nothing, and the wrapper is `v-if="agentAsks.length"`, so the third of ent#364's three surfaces silently did not exist. Verified with a throwaway probe before believing it: asksForAgent('scout') -> 1 ask asksForAgent({name: 'scout'}) -> 0 asks Pinned against the SOURCE, because this repo's vitest runs in `node` with no component-mount harness — crude, but it is the difference between catching this class again and not. **AC #3 — expiry now says WHEN.** The row said "This expired before it was answered" with no time, and the #1142 sweep DELETES terminal rows, so between lapsing and being swept this is the only evidence the question was ever asked. An hour ago and last March are different situations. `expiredLabel` lives in `portalUtils` with `now` injected, per this file's rule that a sentence composed inside a component is a sentence no test can reach; it degrades to the bare wording on a missing, garbled or future timestamp, because a wrong time is worse than no time — the reader would act on it. Plus `askThreadLink`: "Open the conversation", shown only when the ask names a thread the reader is not already in. Additive — it never hides an ask from the thread being read. Narrowing the inline surface to only its own thread is the open question, not this. All Workspace work in the OSS repo, per the standing rule. Backend slice: 687 passed, 1 skipped. Frontend: 52 files / 1097 tests, tokens OK, build OK. 6 of the 9 new backend cases fail without this change; the other 3 are regression guards for properties it could have broken. Related to ent#429. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing said it was (ent#429) Review follow-up. `_clamp_ingested_item` is named and documented as a field-hygiene clamp — "Returns a NEW dict; never mutates the caller's request" — and a reader reasonably concludes it is safe to call speculatively, just to see what a clamped item would look like. It is not, and has not been since ent#364: it resolves `addressed_to_email` against the agent's roster, which is a DB READ and an authorization decision. ent#429 went further and gave it a DB WRITE — `_workspace_thread_for` may create a portal session. That is fine where it is. The write has to precede the context size check, because the thread id is written INTO context and adding it afterwards would append bytes the cap had already signed off on. What is not fine is a docstring that describes the opposite, so it now names both effects before the field list, and says what the second caller would cost: an empty client thread left behind by a clamp whose result was never created. No behaviour change. Slice: 687 passed, 1 skipped. Related to ent#429. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1cc5a7a to
71dd7c5
Compare
vybe
left a comment
There was a problem hiding this comment.
Validated after two rebases onto dev. All 22 non-skipped checks pass; regression diff clean across all six seeds (base 12156 → head 12195, 0 failures either side, No new failures). e2e ran and passed (5m6s) despite the absent ui label, so the frontend half is exercised — better than the merge gate requires.
Packaging. src/backend/client_portal/asks/ is a nested package under client_portal/, which docker/backend/Dockerfile already COPYs as a directory (recursively). So no Dockerfile change is needed — and this is worth stating explicitly, because the sibling case one PR over (shared_sessions, a new top-level package in #2355) did need a line and crash-looped prod-image-smoke without it. Top-level packages are listed one per line; nested ones ride their parent. prod-image-smoke green here confirms it.
Substance. The read path for operator_queue.addressed_to_email, plus the two readers that needed it — and the honest part of this PR is that one of them was leaking: the agent page listed every client's asks rather than the viewer's. list_items growing an addressed_to_email SQL condition is correctly framed as a correctness change, not a performance one — the listing orders by status → priority → age and applies limit before the caller sees a row, so filtering in the caller means "the newest N pending items, then discard", which silently drops a client's older ask behind an unrelated flood. Filtering in SQL is the only shape that returns the right rows.
Tests: test_ent428_workspace_asks.py, test_ent364_addressed_asks.py, test_ent360_workspace_agent_page.py (the leak's named regression), plus workspaceAsks.spec.js on the frontend. architecture.md updated.
Rebase note. Conflicted twice on docs/memory/learnings.md — after #2333 and again after #2357 — both pure append-vs-append at the same insertion point. Resolved keep-both, ordered by entry date. Verified the resulting out-of-order positions are byte-identical to origin/dev's own (the file has pre-existing ordering breaks at 2026-07-08/07 and 07-15/12 among others), so the resolution added entries without perturbing anything.
Security scan clean (Client@Example.COM is a test fixture). No new env var, no host paths, no mode flips.
Cross-tracker note: references ent#428/ent#429 with no closing keyword, and the automation is same-repo only — so the private issues need their status-in-dev label set by hand. Doing that now.
…2365 One conflict: src/backend/main.py router mounts — both sides appended at the same point (rooms routers from main's ent#443, portal_asks_router from dev's ent#428). Both kept; both mount before register_enterprise(app). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The OSS half of ent#428 — the read path for
operator_queue.addressed_to_email(shipped in #2300), and the two readers that needed it. One of those turned out to be leaking.Two commits
1. The primitive.
list_itemsgrows an optionaladdressed_to_emailSQL condition.Not a performance change.
list_itemsorders by status → priority → age and applieslimitbefore the caller sees a row, so filtering the addressee out of the result means "the newest N pending items in the FLEET, of which some happen to be yours". One person's low-priority ask leaves the surface that addressed it as soon as the fleet is busy, while still sittingpending— and by construction nobody else may answer it.2. The leak
/reviewfound.client_portal/agent_page.py::_asks— the client-facing "Waiting on you" block — was scoped byagent_namealone. One agent is routinely shared with several people, so that scope was never the audience; it was the only scope available before ent#364 added the addressee. Reproduced before it was believed:Two clients co-shared on one agent; the second reads the first's ask verbatim. The file already withheld
contextas a known leak surface, which is what makes it deceptive — it looks like a reader that has thought about disclosure.title/question/optionsare agent-authored free text and are exactly where an ask meant for someone else says something not meant for this reader.Pre-existing on
devsince ent#364 — that change added the column, its validation and three renderings that honour it, and never taught this older reader. Fixed here rather than filed, because this is the PR that adds the primitive it needs.Decisions a reviewer should actually check
viewer_emailis required, not defaulted. A default is how the next caller silently gets the unfiltered list back. The twotest_ent360failures this caused were signature-only — both stub the query out, so their subjects (alert-type exclusion,contextnever forwarded) are untouched.list_itemsuses truthiness, so a falsy value means "don't filter" — for this argument that would turn a caller which lost its email into one that sees everyone's asks.Nonestill means no filter (the operator listing);""matches nothing. The divergence is commented at the condition, since it reads like an inconsistency otherwise._validated_addressee), so every stored value is already lower — butcreate_itemis a public writer and a read must not silently depend on every future caller remembering that.respond_to_item(responded_by_id)annotatedOptional[str]. It is ausersid, and an ask answered by someone with no platform account has no row there. NULL id + the answering email is what distinguishes "answered by a client" from "answered by an operator whose account was since deleted", which keeps its id. No behaviour change — nothing in this repo passesNoneyet.Third commit — the module itself is OSS now
workspace_askswas built as an entitled private module. On the standing rule that everything about the Workspace lives in OSS, it moved here:src/backend/client_portal/asks/, mounted unconditionally inmain.py, norequires_entitlement, no registered feature id. Same reasoning as the Workspace itself (ent#356) and rooms (ent#443) —PortalAsks.vue,store.fetchAsks()andstore.asksAvailableship in every build and self-disable on a 404, so a community install rendered the ask affordance and then refused it.Under
client_portal/rather than a new top-level package: it is a client-portal surface, on the client-portal prefix, owning no table — and it inheritsclient_portal's Dockerfile COPY line instead of needing its own. A new top-level package thatmain.pyimports at module scope builds a clean image and dies at import; that has now been #1033, ent#356 and ent#443.Verbatim port — service and models unchanged, router loses its entitlement dependency, service loses
FEATURE_ID. The/api/enterprise/client-portal/asksprefix stays: ent#83 published it as the headless integration surface and the shipped bundle calls it, so renaming breaks a client for nothing.Transition-safe, proven live on a pure-OSS build with an un-bumped submodule — both routers mount and the ungated OSS one wins:
The suite moves with it (
tests/unit/test_ent428_workspace_asks.py, 22 cases), unchanged beyond import paths and one path calculation, plus three the ported ones structurally cannot see: the routes answer with no entitlement; nothing callsrequires_entitlement/register_module(parsed, not grepped — a docstring may still say it used to be gated); the mount precedes the seam.That last one is anchored on the call, not
str.indexof the name —main.pymentionsregister_enterprise(app)in prose above it, so a substring match finds the comment, and it false-failed here on the first run. #2355 carries the same weaker form; it can only false-fail there, never false-pass, and is worth tightening separately.Not touched
No migration, no route signature change, no new top-level package (so no Dockerfile change). One frontend file is touched — comments only, correcting two that explained the 404/403 guard as "the module is enterprise-gated"; the guard itself still belongs, for an older backend. Both OSS operator-queue routes pass explicit kwargs (
routers/operator_queue.py:73,:296), so the new parameter is unreachable from a query string and a platform caller's view of the queue does not move — pinned bytest_no_addressee_argument_leaves_the_operator_listing_alone, which is green with and without this diff, deliberately.Tests
Seven new cases in
tests/unit/test_ent364_addressed_asks.py. Six fail without the source change, verified by stashing it:The crowd-out test asserts up front that its own fixture really pushed the ask out of the unfiltered window — a crowd-out test whose fixture doesn't crowd out passes for the wrong reason (my first attempt did exactly that).
Local: portal/queue slice → 678 passed, 1 skipped; guard sweep (
centralized|parity|invariant|architecture|wiring|1310|186|293|926|enumeration) → 816 passed, 3 skipped.Also exercised end-to-end against a real running instance — a pure-OSS build (
edition=oss,enterprise_features=[]), two clients signed in through the actual OTP flow, asks raised through the real ingestion boundary. Alice sees hers on both surfaces, bob sees[]on both, the seededsk-live-…incontextappears 0 times,bob answers alice's ask → 404,alice → 200,alice again → 400, audit rowresponded_by_id=None / responded_by_email=alice@…, and revoking alice's share mid-session emptied her list with no restart. The operator's own/api/operator-queueview was unmoved.Follow-ups recorded, not silently dropped
docs/memory/learnings.md— the durable class: a column introduced to narrow who sees a row obliges you to grep every existing reader of that table, because a reader written before the column either filters or discloses, and doing nothing is a choice for the second.debt:2026-08-21-workspace-agent-page-duplicate-asks— on an entitled install the agent page now renders the same ask twice (the gated answerable block, and this read-only one). Pre-existing and merely exact now; which block survives is a frontend/product call, so it is not folded into a backend PR.Merge order
No longer coupled.
trinity-enterprise#427reduces to the design of record (its implementation is here now), so it neither blocks nor is blocked; the submodule bump only picks up a removed registration, and nothing breaks if it lags, because the OSS routes win the match order either way.Related to ent#428.
🤖 Generated with Claude Code
Second review pass
/reviewrun again after the module move found four more, all created or exposed by the move rather than present in the ported logic — every one fixed in-branch:architecture.md:1241called the surface "the entitledworkspace_asksmodule". A public doc calling a public module paid is the wrong direction of wrong.stores/clientPortal.jsexplained its 404/403 guard as "the module is enterprise-gated". Guard still belongs (older backend); the stated reason would have told a reader asks are paid. Comments only.asks/models.pystill saidPrivate.list_askscalled the roster check per item, which is 1–2 DB reads each, for an answer that cannot change within one request, on an endpoint polled every 20s per client per tab.The first attempt at (4) was wrong and the tests said so: hoisting
roster_agent_namesout of the loop turned six red, because the suite's seam isagent_on_roster— which is the access predicate, and re-implementing membership beside it is how the two drift. Redone as a per-request memo over that same predicate. Pinned on call count, not timing, and non-vacuous: