diff --git a/docs/memory/architecture/api-endpoints.md b/docs/memory/architecture/api-endpoints.md index cfc85a72fc..4c5a1e4ab9 100644 --- a/docs/memory/architecture/api-endpoints.md +++ b/docs/memory/architecture/api-endpoints.md @@ -267,7 +267,7 @@ Bulk ops scope writes to the caller's accessible agents (tri-state: admin = no f WebSocket events: `operator_queue_new`, `operator_queue_responded` / `operator_queue_cancelled` (thin `{id, agent_name}` since trinity-enterprise#611 — listeners refetch), `operator_queue_acknowledged`, `operator_queue_cleared` (one per bulk op, #1017; count only — `notifications_cleared` for the notifications variant), `operator_queue_sync` (#2915, one per poll cycle; announces expiry too). Backed by the 5s Operator Queue Sync background service. Every ending — answer, cancel, bulk cancel, expiry — goes through one sink, `services/ask_service.py` (trinity-enterprise#611: CAS + endings ledger → one audit row per transition → one thin trigger → the registered ending observers); the machine keys never see the new person fields (`disposed_by_email`, `resolved_to`) on get/list. See [operating-room.md → Endings](../feature-flows/operating-room.md#endings-trinity-enterprise611). -**Addressed asks (ent#364).** `operator_queue.addressed_to_email` names the HUMAN an item is for; `agent_name` only ever said which agent. NULL — every pre-ent#364 row — means "the operator answers", so the column is additive by construction. It is a **column and not a key in `context`** because it is an authorization decision (who may answer, whose Workspace sidebar it appears in) and `context` is agent-authored JSON whose clamp bounds size and type only. Validated at the one agent-authored boundary (`operator_queue_service._validated_addressee`, inside the #1632 clamp) against `client_portal.service.agent_on_roster(agent, email, include_owned=False)` — an agent may address only someone already on its roster — and **fails closed**: malformed, off-roster, or an unreadable roster all drop to NULL. The read/answer surface is **OSS core** (ent#428): `client_portal/asks/`, mounted unconditionally in `main.py` at `/api/enterprise/client-portal/asks` — a prefix that is retained history, like the `enterprise_`-prefixed portal tables, not a licensing claim. It re-checks the roster at read time so a revoked share stops showing an already-raised ask, projects an explicit client-facing shape (never `context`), and answers through the same ask sink as the operator respond route (`services/ask_service.py`, trinity-enterprise#611), so the ledger, write-back, audit and the WS trigger are unforked, and it is person-only like respond (a non-person platform principal gets 403 `person_required`). `responded_by_id` stays NULL for a client answerer — there is no `users` row, and inventing one would be a lie in the audit trail. +**Addressed asks (ent#364).** `operator_queue.addressed_to_email` names the HUMAN an item is for; `agent_name` only ever said which agent. NULL — every pre-ent#364 row — means "the operator answers", so the column is additive by construction. It is a **column and not a key in `context`** because it is an authorization decision (who may answer, whose Workspace sidebar it appears in) and `context` is agent-authored JSON whose clamp bounds size and type only. Validated at the one agent-authored boundary (`operator_queue_service._validated_addressee`, inside the #1632 clamp) against `client_portal.service.agent_on_roster(agent, email, include_owned=False)` — an agent may address only someone already on its roster — and **fails closed**: malformed, off-roster, or an unreadable roster all drop to NULL. The read/answer surface is **OSS core** (ent#428): `client_portal/asks/`, mounted unconditionally in `main.py` at `/api/enterprise/client-portal/asks` — a prefix that is retained history, like the `enterprise_`-prefixed portal tables, not a licensing claim. It re-checks the roster at read time so a revoked share stops showing an already-raised ask, projects an explicit client-facing shape (never `context`), and answers through the same ask sink as the operator respond route (`services/ask_service.py`, trinity-enterprise#611), so the ledger, write-back, audit and the WS trigger are unforked, and it is person-only like respond (a non-person platform principal gets 403 `person_required`). `responded_by_id` stays NULL for a client answerer — there is no `users` row, and inventing one would be a lie in the audit trail. **The list fails loud (trinity-enterprise#610 PR A0):** `GET /api/enterprise/client-portal/asks` returns **503 `{code: "asks_unavailable", message}`** with `Retry-After: 20` (the Workspace poll interval) when any of its queue reads raises (the agent set, the count or the page, #3059) or the read-time roster re-check cannot read the roster (`asks/service.AsksUnavailable`, strict `_on_roster`); it never answers an outage with `[]`. `answer_ask`'s roster failure stays the uniform 404. **Ingestion caps (#1632).** There is **no HTTP create** — every operator-queue item is created through `db.create_operator_queue_item`, and the only untrusted producer is the sync service reading the agent-authored `~/.trinity/operator-queue.json` (`operator_queue_service._sync_agent`). #1402 makes this queue the approval channel for irreversible actions, so a compromised / prompt-injected agent that floods plausible "approve this" items causes operator fatigue → reflexive approval (XSS is already handled by DOMPurify; the exposure is volume + social engineering). The one agent-authored seam is bounded by two independent limits plus field hygiene, all env-tunable (see requirements §26.7): (1) a per-agent **pending-DEPTH cap** (`db.count_operator_queue_pending_for_agent`, default 25) — the **primary** bound, DB-measured ⇒ Redis-independent; at the cap the sync **stops** ingesting (never drips a growing file) and holds the surplus behind one aggregated alert; (2) a per-agent + fleet **create RATE limit** (`rate_limiter.check`, 60/60s + 300/60s, fail-open); (3) a total field-hygiene clamp (`_clamp_ingested_item`, inside the #1525 create try/except) — truncate-with-marker title/question, context/options over cap → marker (validated `execution_id` only), non-dict context → `{}`, `created_at` normalized to ingest time, priority validate-only; (4) a **reserved-id guard** rejecting agent ids that start with a platform-reserved prefix (`queue-flood-`/`poison-`/`cb-dormant-`/`sync-failing-`/`git-bloat-`/`skill-not-found-`/`val_`/`system-seed-`/`base-image-stale-`/`alert-budget-`) so an agent can't pre-create — and via `on_conflict_do_nothing` silence — its own flood alarm or the #1402 poison alert, plus a malformed/oversize-id reject; (5) a per-cycle scan bound (skip an oversized file wholesale; cap at 500 requests/cycle). The 5s sync loop is **leader-locked** (`opqueue:leader`, mirror monitoring #1464) so `--workers 2` doesn't double-charge / double-broadcast / double-scan. Platform creates bypass `_sync_agent`, and the exemption is **scoped by influence, not by caller location (#1677)**: a *platform-only* emitter (volume bound by platform cadence — edge-triggered, idempotent/bucketed id, leader-locked, or operator-driven: lease-reaper poison-park #1402, `validation_service._notify_operator_on_failure` — converted to a **direct DB create** in #1632 — and the internal breaker/skill/git alerts) stays direct and unthrottled, while an *agent-influenceable* one — `task_execution_service._alert_skill_not_found` (#1410), whose per-command dedup distinct unknown slash-commands defeat at $0/turn — routes through `operator_queue_service.create_bounded_alert`: a per-(agent, registered-type) pending-DEPTH budget (`OPERATOR_ALERT_MAX_PENDING_PER_TYPE`, default 5; type derived from `item["type"]` against the `_BUDGETED_ALERT_TYPES` frozenset; **fail-closed on every arm** — an unreadable count / unregistered type / failed create suppresses the ALERT only, the FAILED execution rows stay the primary surface, and the paired notification is gated on the same bool), with ONE cooldown-gated `alert-budget-{agent}-{type}-b{bucket}` episode alert per window (deterministic bucket ⇒ the `(agent_name, request_id)` on-conflict target dedups cross-worker; no `held` count, no agent-controlled text). The classification is CI-forced by the AST caller-parity guard `tests/unit/test_1677_operator_alert_emitters.py` (every `create_operator_queue_item` call site must be allowlisted platform-only or routed; OSS tree only — the private submodule owns its twin); a sink-level default bound was rejected because it fails QUIETLY at the load-bearing poison-park create where the parity test fails LOUDLY at CI (#1890's lesson deliberately inverted). A generous DB-sink belt in `create_item` (reject title>4 KiB / question>16 KiB / context>64 KiB / id>512; the derived `execution_id` COLUMN belted to None on non-str/>512, #1677) is a second layer so the exemption is never solely load-bearing (#1525 validate-at-boundary-AND-at-sink). A depth-held / rate-skipped / oversize-file episode emits **one** `queue-flood-{agent}-{utc_now_iso()}` alert (un-guessable id, priority `high`, in-memory cooldown, emit-failure-safe). The platform-emitter residual named on the pull-mode default-ON gate list (#1081 / `TARGET_ARCHITECTURE.md`) is closed by #1677; the remaining gate items are unchanged. diff --git a/docs/memory/feature-flows/workspace-agent-page.md b/docs/memory/feature-flows/workspace-agent-page.md index 9cc811633f..2cefdc8737 100644 --- a/docs/memory/feature-flows/workspace-agent-page.md +++ b/docs/memory/feature-flows/workspace-agent-page.md @@ -364,6 +364,23 @@ was already live: `_asks` excluded `alert` items while `/asks` includes them, so the page already showed them through ``; only the duplicate hid them. +**An outage is not "nothing waiting" (trinity-enterprise#610 PR A0).** +"Unreadable-roster fail-closed" above used to mean the ask silently vanished: +`list_asks` caught every error and returned `[]`, and `fetchAsks` set +`asks = []` and `asksAvailable = false` on any failure, so a 5xx cleared this +section and the sidebar badge alike. Now the list raises `AsksUnavailable` (a +queue-read fault, or a roster that cannot be READ — an agent genuinely off the +roster is still dropped) and the route answers **503 `asks_unavailable`**. The +store keeps the last good list on any non-404/403 failure, sets `asksFailed`, +leaves `asksAvailable` as it was, and latches `asksLoaded` on the first success +— so `` keeps rendering what it last knew instead of blanking. +A **401** is not an outage but the end of the session: `fetchAsks` calls +`endSession({expired: true, resumePath})` exactly as the roster fetch does +(ent#375), because the 20s poll never re-reads the roster and `portalHttp`'s 401 +interceptor acts only for a platform session. A read that resolves after its +session ended (sign-out, expiry, another client) is dropped, never written. +The 503 carries `Retry-After: 20`. + ### The Overview row is unconditional, and asks moved below it (#2169) #2161 put asks and recent work in a grid whose column count was **bound to @@ -558,6 +575,21 @@ destination" is finally true. ## Tests +`tests/unit/test_ent610_asks_unavailable.py` (trinity-enterprise#610 PR A0) — the +asks list fails loud: a queue-read fault and an unreadable roster each raise +`AsksUnavailable` and answer **503 `asks_unavailable`**; a clean off-roster agent +is still dropped with a 200; `answer_ask` stays a uniform 404 on a roster outage; +the suggestions build still degrades to no ask ids (a guard for the new raise +path, not a regression test: it also passes on the pre-fix code). The raise and +roster halves are mutation-tested (revert the raise to `return []`; make +`_on_roster` swallow again → red). +`src/frontend/tests/unit/workspaceAsks.spec.js` pins the store half: a 5xx keeps +the last good list with `asksFailed` set and `asksAvailable` untouched, a 404 +is absence (and resets `asksLoaded`), a success clears the failure, a +sign-out drops the kept list so the next client never inherits it, a 401 ends a +portal-token session and drops the list, and a read resolving after sign-out +writes nothing (each mutation-tested). + `tests/unit/test_ent360_workspace_agent_page.py` — the projections (no message/cost/model; alerts excluded; `context` never present, asserted against the rendered repr too), cross-agent report isolation and its 404 uniformity, diff --git a/docs/memory/requirements/security.md b/docs/memory/requirements/security.md index 4cfc3d9fe3..63283b393b 100644 --- a/docs/memory/requirements/security.md +++ b/docs/memory/requirements/security.md @@ -564,8 +564,9 @@ - **Escalation = a visible count:** the list response carries `undelivered_count` and `closed_by_filer_count`; the Operations header renders both and offers one-click cancel of `closed_by_filer` rows through the existing bulk-cancel. No new queue item is minted about queue items. - **Audit** (`platform_audit_service`, `event_type=operator_queue`, `source=system`, target = the row uuid, details = ids and enums only, never agent text): `ingested` · `diverged` (→ changed / closed_by_filer / missing / stale_id) · `reconciled` (diverged → confirmed) · `written_back` · `undeliverable`. The `confirmed ↔ unconfirmed` flap is **never** audited (a fleet restart writes zero rows); the edge is the UPDATE's rowcount under `col IS NULL OR col != :v`, so two overlapping leaders cannot double-write. - **Surfaces:** `QueueCard` / `ResolvedCard` (Operations), `/m` (MobileAdmin), and `PortalAsks` render ONE shared rule (`utils/operatorQueue.js::queueSyncBadge`); the portal projection exposes a coarse `sync ∈ {confirmed, changed, closed, unconfirmed}` + `aging` only — no reason, no timestamp. MCP `list_operator_queue` passes the fields through. -- **Files**: `src/backend/services/operator_queue_service.py`, `src/backend/db/operator_queue.py`, `src/backend/routers/operator_queue.py`, `src/backend/client_portal/asks/{models,service}.py`, `src/backend/services/agent_client/client.py`, `docker/base-image/agent_server/routers/files.py`, `src/backend/config.py`, `src/frontend/src/utils/operatorQueue.js` + the four callers, migrations `operator_queue_sync_state` (SQLite) / `0073_operator_queue_sync_state` (Alembic) -- **Tests**: `tests/unit/test_2915_operator_queue_sync_honesty.py`, `tests/unit/test_2915_agent_server_files_if_match.py`, `src/frontend/tests/unit/operatorQueueSyncBadge.spec.js`, `src/frontend/tests/unit/queueCardSyncBadge.spec.js` + - **The Workspace asks read fails loud, never empty (trinity-enterprise#610 PR A0):** `GET /api/enterprise/client-portal/asks` answers **503 `{code: "asks_unavailable"}`** when the queue read fails **or** the roster cannot be read (in list mode an unreadable roster is an outage, not "off the roster"; a genuinely off-roster agent is still dropped silently, and `answer_ask` keeps its uniform 404). The 503 carries `Retry-After: 20`. The store (`stores/clientPortal.js::fetchAsks`) treats only 404/403 as absence and a 401 on a portal-token session as expiry (`endSession({expired: true})`, the roster fetch's own handling, which clears the list); any other failure sets `asksFailed`, **keeps the last good list** and leaves `asksAvailable` untouched, and `asksLoaded` latches on the first success (a 404/403 resets it); `signOut` clears the kept list and all three flags so the next client never inherits it, and a read that resolves after its session ended is dropped — so a 5xx can no longer read as "nothing needs you". This deliberately reverses the pre-#610 behaviour of clearing the list on a real failure (the contract's "never overwrite the data with a synthetic empty payload in a catch", ent#253). The suggestions build keeps degrading through its own `attempt(..., [])`. +- **Files**: `src/backend/services/operator_queue_service.py`, `src/backend/db/operator_queue.py`, `src/backend/routers/operator_queue.py`, `src/backend/client_portal/asks/{models,service,router}.py`, `src/backend/services/agent_client/client.py`, `docker/base-image/agent_server/routers/files.py`, `src/backend/config.py`, `src/frontend/src/utils/operatorQueue.js` + the four callers, `src/frontend/src/stores/clientPortal.js` (`fetchAsks`, `signOut`), migrations `operator_queue_sync_state` (SQLite) / `0073_operator_queue_sync_state` (Alembic) +- **Tests**: `tests/unit/test_2915_operator_queue_sync_honesty.py`, `tests/unit/test_2915_agent_server_files_if_match.py`, `src/frontend/tests/unit/operatorQueueSyncBadge.spec.js`, `src/frontend/tests/unit/queueCardSyncBadge.spec.js`, `tests/unit/test_ent610_asks_unavailable.py`, `src/frontend/tests/unit/workspaceAsks.spec.js` - **Residuals (registered)**: `closed_by_filer` rows still count toward the depth cap until cancelled (trinity-enterprise#619); a response undelivered because the entry changed *after* the answer has no re-answer path (ent#619/#611 — narrowed by OPS-001-ENDINGS: the agent reads the answer back by its own `request_id` over MCP, so a refused file write-back no longer leaves it without the decision); a re-ask with a reused id after `acknowledged` is swallowed by #1631's design (ent#619). - **Env knobs** (all env-tunable, generous by design — "cap a hostile/runaway agent, not throttle normal use"): `OPERATOR_QUEUE_CREATE_RATE_LIMIT` (60), `OPERATOR_QUEUE_CREATE_RATE_WINDOW` (60), `OPERATOR_QUEUE_FLEET_CREATE_RATE_LIMIT` (300), `OPERATOR_QUEUE_MAX_PENDING_PER_AGENT` (25), `OPERATOR_QUEUE_MAX_SCAN_PER_CYCLE` (500), `OPERATOR_QUEUE_MAX_FILE_BYTES` (2 MiB), `OPERATOR_QUEUE_TITLE_MAX` (300), `OPERATOR_QUEUE_QUESTION_MAX` (4000), `OPERATOR_QUEUE_CONTEXT_MAX_BYTES` (8192), `OPERATOR_QUEUE_OPTIONS_MAX_BYTES` (4096), `OPERATOR_QUEUE_ID_MAX` (256), `OPERATOR_QUEUE_EXECUTION_ID_MAX` (128), `OPERATOR_QUEUE_FLOOD_ALERT_COOLDOWN_SECONDS` (300), `OPERATOR_ALERT_MAX_PENDING_PER_TYPE` (5, #1677). - **Files**: `src/backend/services/operator_queue_service.py`, `src/backend/db/operator_queue.py`, `src/backend/services/validation_service.py`, `src/backend/services/task_execution_service.py` (#1677), `src/backend/database.py` diff --git a/src/backend/client_portal/asks/router.py b/src/backend/client_portal/asks/router.py index 3b1481b689..98d443b6b9 100644 --- a/src/backend/client_portal/asks/router.py +++ b/src/backend/client_portal/asks/router.py @@ -22,7 +22,12 @@ from . import service from .models import WorkspaceAsk, WorkspaceAskAnswer -from .service import AskError +from .service import AskError, AsksUnavailable + +# The 503 `asks_unavailable` Retry-After, in seconds (trinity-enterprise#610, +# PR A0). It is the Workspace's asks poll interval (`Portal.vue::ASKS_POLL_MS`): +# its next read is the retry anyway. +ASKS_RETRY_AFTER_SECONDS = "20" router = APIRouter( prefix="/api/enterprise/client-portal/asks", @@ -55,10 +60,19 @@ def list_asks( stops being true. `include_ended` (trinity-enterprise#611) adds the asks that ended in the last 7 days, so a person sees how an ask ended instead of watching it vanish; the sidebar count stays pending-only on the client. + + An unreadable queue or roster is **503 `asks_unavailable`**, never `[]` + (trinity-enterprise#610, PR A0): an empty list is a claim that nothing is + waiting, and the client keeps its last good list on this answer. """ try: page = service.list_asks_page(principal.email, principal.is_platform, agent_name, include_ended=include_ended, limit=limit, cursor=cursor) + except AsksUnavailable: + raise HTTPException(status_code=503, detail={ + "code": "asks_unavailable", + "message": "Couldn't load your asks — try again.", + }, headers={"Retry-After": ASKS_RETRY_AFTER_SECONDS}) except AskError as e: _raise(e) response.headers["X-Total-Count"] = str(page.total) diff --git a/src/backend/client_portal/asks/service.py b/src/backend/client_portal/asks/service.py index f3bffd43ce..aa57700e75 100644 --- a/src/backend/client_portal/asks/service.py +++ b/src/backend/client_portal/asks/service.py @@ -29,6 +29,15 @@ _VISIBLE_KINDS = ("question", "approval", "alert") +class AsksUnavailable(Exception): + """The asks list could not be READ (trinity-enterprise#610, PR A0). + + Raised instead of returning `[]`: an empty list is a claim ("nothing needs + you"), and making it during an outage is the #2915 failure class. The router + maps it to 503 `asks_unavailable`; the store keeps its last good list. + """ + + class AskError(Exception): """A named, actionable refusal — never a bare 422 from a validator.""" @@ -160,19 +169,28 @@ def _coarse_sync(item: dict) -> str: return "unconfirmed" -def _on_roster(agent_name: str, email: str, is_platform: bool) -> bool: +def _on_roster(agent_name: str, email: str, is_platform: bool, + strict: bool = False) -> bool: """Read-time roster re-check (ent#364). Membership at raise time is not a standing grant: a share revoked afterwards - must stop showing the ask. Fails CLOSED — an unreadable roster hides the ask - rather than showing one we cannot justify. + must stop showing the ask. Fails CLOSED — an unreadable roster never shows an + ask we cannot justify. + + `strict` is the LIST mode (trinity-enterprise#610, PR A0): there an + unreadable roster raises `AsksUnavailable`, because hiding every ask during a + roster outage is a silent "nothing needs you". A clean "not on the roster" + still returns False in both modes. The answer path stays non-strict, so its + refusal is the uniform 404 whatever the cause (Invariant #8). """ try: from client_portal.service import agent_on_roster return bool(agent_on_roster(agent_name, email, include_owned=is_platform)) - except Exception: # noqa: BLE001 + except Exception as e: # noqa: BLE001 logger.warning("[WorkspaceAsks] roster re-check failed for %s", agent_name, exc_info=True) + if strict: + raise AsksUnavailable("roster unreadable") from e return False @@ -216,8 +234,12 @@ def list_asks_page(email: str, is_platform: bool, agent_name: Optional[str] = No cursor: Optional[str] = None) -> AsksPage: """Asks addressed to `email`: open ones first, then — with `include_ended` — the ones that ended in the last `ENDED_WINDOW_DAYS`, most recent ending - first (trinity-enterprise#611). Paged (#3059). Never raises on a read - failure; raises `AskError` only for an unreadable cursor.""" + first (trinity-enterprise#611). Paged (#3059). + + Raises `AskError` for an unreadable cursor, and `AsksUnavailable` when the + queue or the roster cannot be read (trinity-enterprise#610, PR A0) — an + outage is never answered with an empty page, which the client would render + as "nothing is waiting".""" offset = _decode_cursor(cursor) limit = max(1, min(int(limit), PAGE_MAX)) # Every filter the viewer's visibility depends on is a SQL condition, @@ -243,15 +265,15 @@ def list_asks_page(email: str, is_platform: bool, agent_name: Optional[str] = No # agents the viewer's asks span, and the answer goes into the SQL as the # access set. Re-implementing membership here is how the two drift. allowed = {a for a in db.list_operator_queue_agent_names(**filters) - if _on_roster(a, email, is_platform)} + if _on_roster(a, email, is_platform, strict=True)} if not allowed: return AsksPage() total = db.count_operator_queue_items(accessible_agent_names=allowed, **filters) items = db.list_operator_queue_items(accessible_agent_names=allowed, limit=limit, offset=offset, **filters) - except Exception: # noqa: BLE001 — a sidebar badge must not break the Workspace + except Exception as e: # noqa: BLE001 — surfaced as a 503, never as "nothing" (A0) logger.warning("[WorkspaceAsks] list failed", exc_info=True) - return AsksPage() + raise AsksUnavailable("queue unreadable") from e out = [_project(item, viewer_email=email) for item in items or []] end = offset + len(items or []) diff --git a/src/frontend/src/stores/clientPortal.js b/src/frontend/src/stores/clientPortal.js index 5ee495ed69..e287a0d720 100644 --- a/src/frontend/src/stores/clientPortal.js +++ b/src/frontend/src/stores/clientPortal.js @@ -300,6 +300,14 @@ export const useClientPortalStore = defineStore('clientPortal', { // sync step, and three separate queries is how that stops being true. asks: [], asksAvailable: false, // false when the backend does not serve /asks (404/403) + // trinity-enterprise#610 (PR A0): the read's honesty. `asksLoaded` latches on + // the first success; `asksFailed` is the LAST read's verdict for anything but + // 404/403 (a 5xx, 503 `asks_unavailable`, a network error) — the list keeps + // what it last knew, so a surface can say "couldn't refresh" instead of + // claiming nothing is waiting. `asksLoadedAt` feeds the stale banner. + asksLoaded: false, + asksFailed: false, + asksLoadedAt: null, // Where the user was when it expired, so re-authenticating returns them // there instead of the roster root. resumePath: null, @@ -591,6 +599,14 @@ export const useClientPortalStore = defineStore('clientPortal', { // client signing in on the same browser must not inherit this list. this.modelOptions = [] this.rosterLoaded = false + // trinity-enterprise#610 (PR A0): a failed read keeps the last good list, + // so the list must not outlive the session it belongs to — or the next + // client's first failed read shows them the previous client's asks. + this.asks = [] + this.asksAvailable = false + this.asksLoaded = false + this.asksFailed = false + this.asksLoadedAt = null // #2261: the primitive clears the suppression; `endSession({expired})` // re-arms it immediately afterwards. Keeping the clear HERE is what stops // a marker from outliving the session it was about. @@ -2024,13 +2040,33 @@ export const useClientPortalStore = defineStore('clientPortal', { // store onto a dedicated instance so a workspace request can never inherit // the platform JWT from `axios.defaults`. // - // Degrades to silence. Asks are OSS core since ent#428, so a CURRENT backend - // always serves this; against an OLDER one — which either predates the - // surface or still gates it behind the entitlement it used to carry — the + // Absence degrades to silence. Asks are OSS core since ent#428, so a CURRENT + // backend always serves this; against an OLDER one — which either predates + // the surface or still gates it behind the entitlement it used to carry — the // 404/403 is not an error worth showing a client. `asksAvailable` stays // false and every surface renders nothing. + // + // A FAILURE is not absence (trinity-enterprise#610, PR A0). Anything else — + // 5xx, 503 `asks_unavailable`, network — sets `asksFailed`, keeps the last + // good list and leaves `asksAvailable` alone: clearing either would blank + // every PortalAsks surface and zero the badge, i.e. "nothing needs you" + // during an outage (#2915; the contract's "never overwrite the data with a + // synthetic empty payload in a catch", ent#253). + // + // A 401 is neither: the SESSION ended. It goes through `endSession` exactly + // as the roster fetch's 401 does (ent#375), which clears the list via + // `signOut()`. Folding it into `asksFailed` kept the previous client's list + // on screen: the 20s poll runs this and `refreshThreads`, never the roster, + // and `portalHttp`'s 401 interceptor acts only for a platform session — so + // for a portal-token client nothing else would ever end it. + // + // A read that resolves after the session it was issued under has ended + // (sign-out, expiry, another client signing in) is dropped: writing it + // would put that session's asks back into the store the sign-out cleared. async fetchAsks(agentName = null) { if (!this.isClientSignedIn) return [] + const issuedUnder = this.portalToken + const stale = () => this.portalToken !== issuedUnder || !this.isClientSignedIn try { // trinity-enterprise#611: the asks that ended in the last 7 days ride the // same list, so a person sees how an ask ended instead of watching it @@ -2039,16 +2075,32 @@ export const useClientPortalStore = defineStore('clientPortal', { headers: this.authHeader, params: agentName ? { agent_name: agentName, include_ended: true } : { include_ended: true }, }) + if (stale()) return [] this.asks = Array.isArray(data) ? data : [] this.asksAvailable = true + this.asksLoaded = true + this.asksFailed = false + this.asksLoadedAt = Date.now() return this.asks } catch (err) { - this.asksAvailable = false - if (![403, 404].includes(err.response?.status)) { - console.warn('[workspace] asks unavailable:', err?.message || err) + if (stale()) return [] + if (err.response?.status === 401 && this.portalToken) { + this.endSession({ + expired: true, + resumePath: typeof window !== 'undefined' ? window.location.pathname : null, + }) + return [] } - this.asks = [] - return [] + if ([403, 404].includes(err.response?.status)) { + this.asksAvailable = false + this.asksLoaded = false + this.asksFailed = false + this.asks = [] + return [] + } + console.warn('[workspace] asks unavailable:', err?.message || err) + this.asksFailed = true + return this.asks } }, diff --git a/src/frontend/tests/unit/workspaceAsks.spec.js b/src/frontend/tests/unit/workspaceAsks.spec.js index c7abfd43d9..cc6339fdb5 100644 --- a/src/frontend/tests/unit/workspaceAsks.spec.js +++ b/src/frontend/tests/unit/workspaceAsks.spec.js @@ -157,7 +157,14 @@ describe('an unentitled build says nothing', () => { warn.mockRestore() }) - it('a real failure is reported once, and clears the list rather than showing stale asks', async () => { + // trinity-enterprise#610 (PR A0) REVERSED this case. It used to pin "a real + // failure clears the list rather than showing stale asks" — and with + // `asksAvailable=false` that blanked every PortalAsks surface and dropped the + // sidebar badge to zero on a 5xx: the Workspace claiming "nothing needs you" + // during an outage (#2915). The design-system contract forbids exactly that + // ("never overwrite the data with a synthetic empty payload in a catch", + // ent#253). A failure now keeps the last good list and SAYS it failed. + it('a real failure is reported once, keeps the last good list and says it failed', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) portalHttp.get.mockResolvedValueOnce({ data: [ask('a1')] }) await store.fetchAsks() @@ -166,11 +173,147 @@ describe('an unentitled build says nothing', () => { await store.fetchAsks() expect(warn).toHaveBeenCalledTimes(1) + expect(store.asks.map((a) => a.id)).toEqual(['a1']) + expect(store.askCount).toBe(1) + expect(store.asksFailed).toBe(true) + expect(store.asksLoaded).toBe(true) + // Left as it was: flipping it would blank PortalAsks (its render gate). + expect(store.asksAvailable).toBe(true) + warn.mockRestore() + }) + + it('a 503 asks_unavailable or a network error before any success is a failure, not absence', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + portalHttp.get.mockRejectedValueOnce({ + response: { status: 503, data: { detail: { code: 'asks_unavailable' } } }, + }) + await store.fetchAsks() + + expect(store.asksFailed).toBe(true) + expect(store.asksLoaded).toBe(false) + expect(store.asks).toEqual([]) + + portalHttp.get.mockRejectedValueOnce(new Error('Network Error')) + await store.fetchAsks() + expect(store.asksFailed).toBe(true) + expect(store.asksLoaded).toBe(false) + warn.mockRestore() + }) + + it('a success after a failure clears the failure and stamps the load time', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + portalHttp.get.mockRejectedValueOnce({ response: { status: 500 } }) + await store.fetchAsks() + expect(store.asksFailed).toBe(true) + expect(store.asksLoadedAt).toBe(null) + + portalHttp.get.mockResolvedValueOnce({ data: [ask('a2')] }) + await store.fetchAsks() + + expect(store.asksFailed).toBe(false) + expect(store.asksLoaded).toBe(true) + expect(store.asksAvailable).toBe(true) + expect(typeof store.asksLoadedAt).toBe('number') + expect(store.asks.map((a) => a.id)).toEqual(['a2']) + warn.mockRestore() + }) + + it('a 404 after a success is absence again, not a failure', async () => { + portalHttp.get.mockResolvedValueOnce({ data: [ask('a1')] }) + await store.fetchAsks() + + portalHttp.get.mockRejectedValueOnce({ response: { status: 404 } }) + await store.fetchAsks() + + expect(store.asksAvailable).toBe(false) + expect(store.asks).toEqual([]) + expect(store.asksFailed).toBe(false) + // A surface that no longer exists has no "loaded" verdict either. + expect(store.asksLoaded).toBe(false) + }) + + // Keeping the last good list on a failure makes the list session-scoped + // state: it must not outlive a sign-out, or the next client on the same + // browser sees the previous client's asks on their first failed read. + it('a sign-out drops the kept list, so the next client never inherits it', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + portalHttp.get.mockResolvedValueOnce({ data: [ask('a1')] }) + await store.fetchAsks() + expect(store.asks.map((a) => a.id)).toEqual(['a1']) + + store.signOut() expect(store.asks).toEqual([]) expect(store.asksAvailable).toBe(false) + expect(store.asksLoaded).toBe(false) + expect(store.asksFailed).toBe(false) + expect(store.asksLoadedAt).toBe(null) + + store.portalToken = 'another-client' + portalHttp.get.mockRejectedValueOnce({ response: { status: 503 } }) + await store.fetchAsks() + + expect(store.asks).toEqual([]) + expect(store.askCount).toBe(0) + expect(store.asksFailed).toBe(true) warn.mockRestore() }) + // A 401 is not an outage, it is the end of the session. The 20s poll runs + // `fetchAsks`, never the roster, and `portalHttp`'s 401 interceptor acts only + // for a platform session, so for a portal-token client this branch is the + // ONLY thing that ends it. Counted as `asksFailed`, the expired client's list + // stayed on screen indefinitely. + it('a 401 on a portal-token session ends it, as the roster does, and drops the list', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + portalHttp.get.mockResolvedValueOnce({ data: [ask('a1')] }) + await store.fetchAsks() + expect(store.asks.map((a) => a.id)).toEqual(['a1']) + + portalHttp.get.mockRejectedValueOnce({ response: { status: 401 } }) + const out = await store.fetchAsks() + + expect(out).toEqual([]) + expect(store.asks).toEqual([]) + expect(store.askCount).toBe(0) + expect(store.asksFailed).toBe(false) + expect(store.portalToken).toBe(null) + expect(store.isClientSignedIn).toBe(false) + expect(store.sessionExpired).toBe(true) + expect(store.resumePath).toBe(window.location.pathname) + expect(localStorage.getItem('trinity.portalToken')).toBe(null) + // Not an outage: nothing to warn about. + expect(warn).not.toHaveBeenCalled() + warn.mockRestore() + }) + + it('a read that resolves after sign-out does not write the old list back', async () => { + let resolve + portalHttp.get.mockReturnValueOnce(new Promise((r) => { resolve = r })) + const inFlight = store.fetchAsks() + + store.signOut() + resolve({ data: [ask('a1')] }) + await inFlight + + expect(store.asks).toEqual([]) + expect(store.asksLoaded).toBe(false) + expect(store.asksAvailable).toBe(false) + }) + + it('a read that resolves after ANOTHER client signed in is dropped too', async () => { + let resolve + portalHttp.get.mockReturnValueOnce(new Promise((r) => { resolve = r })) + const inFlight = store.fetchAsks() + + store.signOut() + store.portalToken = 'another-client' + resolve({ data: [ask('a1')] }) + await inFlight + + expect(store.asks).toEqual([]) + expect(store.asksLoaded).toBe(false) + }) + it('does not poll at all when nobody is signed in', async () => { localStorage.clear() setActivePinia(createPinia()) diff --git a/tests/registry.json b/tests/registry.json index f4949cd2f8..7519a3b367 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -3659,6 +3659,16 @@ "git" ], "description": "`trinity deploy --repo` creates a deployment (trinity-enterprise#705): the CLI's POST /api/agents payload carries kind \"deployment\", so deploying a codebase never takes the agent default's working branch + auto-push. Static (AST) because the unit job does not install the CLI's own deps. The ownership rule itself (catalog template / owner != token login stays pull-only) is in test_1484_create_agent_characterization.py." + }, + { + "file": "unit/test_ent610_asks_unavailable.py", + "feature": "trinity-enterprise#610", + "added": "2026-09-28", + "categories": [ + "backend", + "workspace" + ], + "description": "Workspace asks read fails loud (ent#610 PR A0): a fault in any of the three queue reads (agent set, count, page — #3059) and an unreadable roster each raise AsksUnavailable and the route answers 503 asks_unavailable with Retry-After: 20 (never []); a genuinely off-roster agent is still dropped with a 200; answer_ask stays a uniform 404 on a roster outage (Invariant #8); the suggestions build still degrades to no ask ids through its own attempt() (a guard for the new raise path; it also passes pre-fix). The app is a module-scoped fixture. Mutation: revert the raise to return an empty page (9 red across this file and test_ent428)." } ] } diff --git a/tests/unit/test_ent428_workspace_asks.py b/tests/unit/test_ent428_workspace_asks.py index 2fc7257e4c..765de703f8 100644 --- a/tests/unit/test_ent428_workspace_asks.py +++ b/tests/unit/test_ent428_workspace_asks.py @@ -174,8 +174,11 @@ def test_a_revoked_share_stops_showing_an_already_raised_ask(asks_db, client_ema assert service.list_asks(client_email, is_platform=False) == [] -def test_an_unreadable_roster_hides_rather_than_shows(asks_db, client_email, monkeypatch): - """Fail closed: showing an ask we cannot justify is worse than hiding one.""" +def test_an_unreadable_roster_shows_nothing_and_says_so(asks_db, client_email, monkeypatch): + """Still fail closed — no ask we cannot justify is shown — but no longer + SILENTLY: trinity-enterprise#610 (PR A0) reversed the old `== []` pin. An + empty list is the claim "nothing needs you", and making it during a roster + outage is the #2915 class; the list raises and the route answers 503.""" import client_portal.service as portal_service from client_portal.asks import service _raise_ask(addressed=client_email) @@ -184,7 +187,8 @@ def boom(*_a, **_kw): raise RuntimeError("roster down") monkeypatch.setattr(portal_service, "agent_on_roster", boom) - assert service.list_asks(client_email, is_platform=False) == [] + with pytest.raises(service.AsksUnavailable): + service.list_asks(client_email, is_platform=False) def test_a_client_read_never_unlocks_the_owned_agents_branch(asks_db, client_email, roster): diff --git a/tests/unit/test_ent610_asks_unavailable.py b/tests/unit/test_ent610_asks_unavailable.py new file mode 100644 index 0000000000..10bcf8da25 --- /dev/null +++ b/tests/unit/test_ent610_asks_unavailable.py @@ -0,0 +1,291 @@ +"""The Workspace asks read fails LOUD, never empty (trinity-enterprise#610, PR A0). + +Before this, `client_portal/asks/service.py::list_asks` caught every error and +returned `[]`, and `_on_roster` turned an unreadable roster into "not on the +roster". Either one made the Workspace say "nothing needs you" during an outage +— the #2915 failure class, and the exact claim the Inbox's Action tab makes. + +Now: + * a queue-read fault raises `AsksUnavailable`, and the route answers + **503 `{code: "asks_unavailable"}`**; + * in LIST mode a roster that cannot be READ is the same outage (503), while an + agent that is genuinely off the roster is still dropped silently (200); + * `answer_ask` keeps its uniform 404 on a roster failure (Invariant #8); + * the suggestions build, the only other backend caller, still degrades through + its own `attempt(..., [])`. +""" + +from __future__ import annotations + +import uuid + +import pytest + +pytestmark = pytest.mark.unit + + +@pytest.fixture() +def asks_db(tmp_path, monkeypatch): + db_file = tmp_path / "trinity-asks.db" + monkeypatch.setenv("TRINITY_DB_PATH", str(db_file)) + + import db.connection as conn_mod + + monkeypatch.setattr(conn_mod, "DB_PATH", str(db_file)) + + import sqlite3 + + from db.schema import init_schema + + raw = sqlite3.connect(db_file) + init_schema(raw.cursor(), raw) + raw.commit() + raw.close() + yield str(db_file) + + +@pytest.fixture() +def client_email(): + # `db.engine` caches the engine at first use, so every test addresses its own + # email rather than relying on a fresh database (see test_ent428). + return f"client-{uuid.uuid4().hex[:8]}@example.com" + + +@pytest.fixture(autouse=True) +def roster(monkeypatch): + """Roster membership, controllable: `on` answers, `raises` is an outage.""" + state = {"on": True, "raises": False} + + import client_portal.service as portal_service + + def _on_roster(agent_name, email, include_owned=False): + if state["raises"]: + raise RuntimeError("roster down") + return state["on"] + + monkeypatch.setattr(portal_service, "agent_on_roster", _on_roster) + return state + + +def _raise_ask(addressed, agent="agent-a"): + from database import db + from services.operator_queue_service import _clamp_ingested_item + + item = _clamp_ingested_item( + { + "id": f"req-{uuid.uuid4().hex[:12]}", + "type": "question", + "title": "Need a decision", + "question": "Ship it?", + "options": ["yes", "no"], + "addressed_to_email": addressed, + }, + agent, + ) + return db.create_operator_queue_item(agent, item) + + +@pytest.fixture(scope="module") +def asks_app(): + """One app over the asks router for the module, built once and torn down with + it; `get_portal_principal` is overridden by walking the routes' own dependant + trees (the test_ent611 pattern, no reimport). The principal is a mutable cell + each test sets through `client_for`, so no module global carries state across + a random test order.""" + from fastapi import FastAPI + + from client_portal.asks import router as ar + + app = FastAPI() + app.include_router(ar.router) + found = set() + + def walk(dependant): + for sub in dependant.dependencies: + if getattr(sub.call, "__name__", "") == "get_portal_principal": + found.add(sub.call) + walk(sub) + + for route in ar.router.routes: + if getattr(route, "dependant", None) is not None: + walk(route.dependant) + assert found, "no get_portal_principal dependency on the asks routes" + principal = {"p": None} + for call in found: + app.dependency_overrides[call] = lambda: principal["p"] + yield app, principal + app.dependency_overrides.clear() + + +@pytest.fixture() +def client_for(asks_app): + from fastapi.testclient import TestClient + + from client_portal.portal_auth import PortalPrincipal + + app, principal = asks_app + + def _make(email): + principal["p"] = PortalPrincipal(email=email, is_platform=False) + return TestClient(app, raise_server_exceptions=False) + + yield _make + principal["p"] = None + + +# Every queue read the list makes (#3059 split one read into three: the agents +# the viewer's asks span, the count, the page). A fault in ANY of them is the +# same outage. An ask is seeded first so the later reads are actually reached. +_QUEUE_READS = ( + "list_operator_queue_agent_names", + "count_operator_queue_items", + "list_operator_queue_items", +) + + +URL = "/api/enterprise/client-portal/asks" + + +# --- the service raises ------------------------------------------------------ + + +@pytest.mark.parametrize("read", _QUEUE_READS) +def test_a_queue_read_fault_raises_rather_than_returning_empty( + asks_db, client_email, monkeypatch, read +): + from client_portal.asks import service + + _raise_ask(client_email) + + def boom(**_kw): + raise RuntimeError("db down") + + monkeypatch.setattr(service.db, read, boom) + with pytest.raises(service.AsksUnavailable): + service.list_asks(client_email, is_platform=False) + + +def test_an_unreadable_roster_raises_in_list_mode(asks_db, client_email, roster): + from client_portal.asks import service + + _raise_ask(client_email) + + roster["raises"] = True + with pytest.raises(service.AsksUnavailable): + service.list_asks(client_email, is_platform=False) + + +def test_an_agent_genuinely_off_the_roster_is_still_dropped_silently( + asks_db, client_email, roster +): + """Fail-closed is unchanged for a CLEAN "no": that is an authorisation answer, + not an outage, so it hides the ask and the list is still a success.""" + from client_portal.asks import service + + _raise_ask(client_email) + + roster["on"] = False + assert service.list_asks(client_email, is_platform=False) == [] + + +def test_answering_on_an_unreadable_roster_stays_a_uniform_404( + asks_db, client_email, roster +): + """Invariant #8: the answer path must not start distinguishing an outage from + not-yours — that would be an existence oracle for ask ids.""" + from client_portal.asks import service + + ask_id = _raise_ask(client_email) + + roster["raises"] = True + with pytest.raises(service.AskError) as exc: + service.answer_ask(ask_id, client_email, False, "yes", None) + assert exc.value.status_code == 404 + assert exc.value.code == "not_found" + + +# --- the route answers 503 ----------------------------------------------------- + + +@pytest.mark.parametrize("read", _QUEUE_READS) +def test_the_route_answers_503_asks_unavailable_on_a_queue_fault( + asks_db, client_email, monkeypatch, client_for, read +): + from client_portal.asks import service + + _raise_ask(client_email) + + def boom(**_kw): + raise RuntimeError("db down") + + monkeypatch.setattr(service.db, read, boom) + r = client_for(client_email).get(URL, params={"include_ended": True}) + + assert r.status_code == 503 + assert r.json()["detail"]["code"] == "asks_unavailable" + assert r.json()["detail"]["message"] + # A back-off hint for clients and proxies; the poll interval, not a default. + assert r.headers.get("Retry-After") == "20" + + +def test_the_route_answers_503_on_an_unreadable_roster( + asks_db, client_email, roster, client_for +): + _raise_ask(client_email) + roster["raises"] = True + + r = client_for(client_email).get(URL) + + assert r.status_code == 503 + assert r.json()["detail"]["code"] == "asks_unavailable" + + +def test_the_route_still_answers_200_for_a_clean_off_roster_agent( + asks_db, client_email, roster, client_for +): + _raise_ask(client_email) + roster["on"] = False + + r = client_for(client_email).get(URL) + + assert r.status_code == 200 + assert r.json() == [] + + +def test_the_route_answers_200_with_the_ask_when_everything_reads( + asks_db, client_email, client_for +): + ask_id = _raise_ask(client_email) + + r = client_for(client_email).get(URL) + + assert r.status_code == 200 + assert [a["id"] for a in r.json()] == [ask_id] + + +# --- the other caller still degrades -------------------------------------------- + + +def test_the_suggestions_build_still_degrades_to_no_ask_ids( + asks_db, client_email, monkeypatch +): + """`suggestions/service._gather` wraps the call in `attempt(..., [])`; a raise + here costs the suggestions their ask items, never the whole list. + + A guard for the new raise path, not a regression test: the pre-fix + `list_asks` returned `[]`, so this passes there too.""" + from datetime import datetime, timezone + + from client_portal.asks import service + from client_portal.suggestions import service as suggestions + + def boom(**_kw): + raise RuntimeError("db down") + + _raise_ask(client_email) + monkeypatch.setattr(service.db, "list_operator_queue_agent_names", boom) + out = suggestions._gather( + "agent-a", client_email, datetime.now(timezone.utc), False + ) + + assert out["ask_ids"] == []