Repository navigation
fix(workspace): the asks read fails loud, never an empty list (ent#610 PR A0) - #3049
Conversation
…PR A0) The requirement (security.md §26.8), the endpoint note and the agent-page flow state the new contract before the code: 503 asks_unavailable on a queue or roster read fault, and a store that keeps the last good list.
…er [] (ent#610 PR A0) list_asks caught every error and returned [], and _on_roster turned an unreadable roster into "not on the roster" — both made the Workspace say "nothing needs you" during an outage (the #2915 class). The list now raises AsksUnavailable (strict roster mode in list only) and the route answers 503. A clean off-roster agent is still dropped; answer_ask keeps its uniform 404. test_ent428's unreadable-roster case pinned the old == [] and is reversed deliberately.
…so (ent#610 PR A0) fetchAsks treated every error as absence: it cleared the list and set asksAvailable=false, blanking every PortalAsks surface and zeroing the badge on a 5xx. Now only 404/403 are absence; any other failure sets asksFailed, keeps the list and leaves asksAvailable alone. asksLoaded latches on the first success and asksLoadedAt feeds a stale banner. workspaceAsks.spec.js's "clears the list rather than showing stale asks" pinned the old behaviour and is reversed deliberately (ent#253).
Keeping the last good list on a failed read made it session-scoped state, but signOut() never cleared it, so on a shared browser the next client's first failed read showed the previous client's asks and badge. signOut now resets asks, asksAvailable, asksLoaded, asksFailed and asksLoadedAt. A 404 also resets asksLoaded: a surface that no longer exists has no verdict. Found by /cso and /review. Mutations (drop the sign-out reset; drop the 404 reset) each turn one spec red.
…ent#610 PR A0) It passes on the pre-fix code too, since the old list_asks returned []. The flow doc, registry entry and docstring now say so. The flow doc also names the sign-out reset.
…(ent#610 PR A0) The #2915 entry's Files and Tests lines now name the asks router, the store, and both A0 test files. The A0 bullet also names the sign-out reset. Found by /validate-pr.
…10-a0 # Conflicts: # tests/registry.json
dolho
left a comment
There was a problem hiding this comment.
Early look, as requested. This is a comment rather than an approval since the PR is still a draft, but it looks approvable as is.
Verification. In a worktree, backend test_ent610_asks_unavailable.py + test_ent428_workspace_asks.py gave 35 passed, and frontend workspaceAsks.spec.js gave 34 passed.
What I checked
- Strict/non-strict split in
_on_roster: this is the right shape. The list read now fails loud, and the answer path keeps its uniform 404 (Invariant #8). - The strict path is reachable:
agent_on_roster→roster_agent_namesdoesn't catch its own errors, so a DB fault really does reach the newexcept. - 503 body: a fixed
{code, message}with no exception text. The details go only to the server log. suggestions/service._gather: still falls back to[]viaattempt(..., []).- Frontend callers:
Portal.vue's poll andPortalAsks.vue's refetch ignore the return value, so keeping the list on failure is safe for them. - Tests: the mutation table in the PR body matches the tests.
Findings
- Minor:
stores/clientPortal.js:2073-2081. An outage on the first read of a page load still renders as "nothing needs you". With no earlier success,asksAvailablestays false andasksstays[]. That is the likeliest outage case, someone reloading during an incident. The PR body says PR A fixes it; please link that follow-up here so the gap has an owner. - Minor:
stores/clientPortal.js:2073. A 401 now counts asasksFailedand keeps the list. That's only safe because the portal's 401 handler ends the session, andsignOut()clears the list. Either handle 401 explicitly or add a test that pins that dependency. - Nit:
fetchAsks(existing behaviour). A request still in flight at sign-out that succeeds afterwards can write the previous client's asks back into the store. Now that the list is kept across failures, a cheap guard is worth it: captureportalTokenbefore theawaitand drop the result if it changed. - Nit:
client_portal/asks/router.py:62. ARetry-Afterheader on the 503 would let clients and proxies back off. - Nit:
tests/unit/test_ent610_asks_unavailable.py:88-122. The module-global_APP/_PRINCIPALcache would read better, and be safer under random test order, as a module-scoped fixture.
|
merge-train: not on train #3070 — rides the next train once fixed.
Otherwise validated clean (backend + store tests execute the real paths; |
There was a problem hiding this comment.
Approving now that this is out of draft. The head is unchanged (c994c6711) since my /review this morning, which found no critical findings. CI is green (22 pass) and it merges cleanly with dev.
The informational items from that review still stand for this PR or its follow-up:
- A first-load outage still reads as "nothing needs you". Link the PR A follow-up that fixes it.
- The 401 path relies on
signOut()clearing the list. Handle it explicitly or pin it with a test. - A
fetchAsksthat resolves after sign-out can write back the previous client's list. Guard it with aportalTokencheck.
One more thing to know: #3077 (the asks read's X-Total-Count / X-Next-Cursor paging) touches list_asks in the same route and service. Whichever lands second resolves a small overlap.
|
Resolve by merging |
…10-a0 # Conflicts: # src/backend/client_portal/asks/router.py # src/backend/client_portal/asks/service.py # tests/registry.json
…ueue read (ent#610 PR A0) The merge with dev brought #3059's paging, which splits the list into three queue reads (agent set, count, page). The fault tests patched only the page read with no ask seeded, so they no longer reached it. They now seed an ask and parametrize over all three reads. The 503 asks_unavailable now carries Retry-After: 20, the Workspace's asks poll interval. The test app moves from module globals to a module-scoped fixture.
A 401 was counted as asksFailed and the list was kept. For a portal-token
client nothing else ends the session: the 20s poll never re-reads the roster,
and portalHttp's 401 interceptor acts only for a platform session. So an
expired client's asks stayed on screen. fetchAsks now calls
endSession({expired: true, resumePath}), the roster fetch's own handling,
which clears the list through signOut().
A read that resolves after its session ended (sign-out, expiry, another
client signing in) is now dropped instead of writing the old list back.
dev carried PR A0's final commits (#3049: the 503 Retry-After, a 401 ends the session, a late read is dropped) that this branch predated. Resolved as the union: A0-final plus the Inbox's asksAbsent verdict, excludeIds/askIds/ testidPrefix props and the session-list sign-out reset. The A0 unit test takes dev's parametrized every-queue-read version.
Refs trinity-enterprise#610
What
PR A0 of #610 (asks honesty). The Workspace asks read used to answer
[]whenever the operator queue or the agent roster couldn't be read, so a DB fault looked exactly like "nothing needs you". Now it fails loudly and the UI keeps the last good answer.Backend (
client_portal/asks/)list_asksraisesAsksUnavailablewhen the queue read fails, instead of returning[]._on_roster(strict=True)(used by list) raises on a roster-read fault. An agent that is cleanly not on the roster is still dropped.AsksUnavailable→ 503{"code": "asks_unavailable", "message": ...}, the same{code, message}shape as the asks router'sAskError.answer_askis unchanged: non-strict, uniform 404 (Invariant security: implement safe tar extraction with symlink/hardlink validation #8).list_askscaller,suggestions/service.py, is wrapped inattempt(..., [])and still degrades.Frontend (
stores/clientPortal.js)fetchAsks: 403/404 is absence, the same as today, and it now also resetsasksLoaded.asksFailed. The last goodaskslist andasksAvailableare kept.asksFailedand stampsasksLoadedAt.signOut()now clears the list and all three flags, so the next client on the same browser never inherits the previous client's asks.What A0 ships on its own:
asksAvailable+asks, which are live today.asksFailed/asksLoadedAtarrives in the next PR of the stack (PR A, the Inbox). Until then the kept list carries no stale marker.Docs:
api-endpoints.mdasks row (503).requirements/security.md: the bug(operator-queue): a pending approval can sit unseen for days — the human's card goes stale while the container↔platform sync stays silent #2915 entry's A0 bullet, plus its Files and Tests lines.feature-flows/workspace-agent-page.md.tests/registry.json.Evidence
Review and security
/review: no blockers. One should-fix:signOutdidn't clear the now-kept list, so on a shared browser the next client's first failed read showed the previous client's asks. Fixed in 9b74ea6. Also fixed: a 404 now resetsasksLoaded, and the suggestions test is relabelled as a guard, not a regression test (cb0c28d). Out of scope: the suggestions payload could also surfaceasks_unavailablelater./cso --diff: it found the same sign-out issue (medium, fixed). It also reports:{code, message}, no exception text);[]./validate-pr: the stale Files/Tests lines in requirements were fixed in 49cb1fc.Mutations. Each change is reverted and the named tests must go red:
return []_on_rosterstrict raise → swallowtest_ent428pin)answer_ask→strict=Trueexcept AsksUnavailableremovedorigin/devsignOutreset removedasksLoadedreset removedFrontend gates (on the head):
npm run test:unit: 175 files, 3684/3684. This includesrawColorRatchetandloadingGateRatchet, and no per-file count grew.npm run build: OK.npm run check:tokens: OK.Browser, both themes.
PortalSidebarandPortalAskswith the real store and axios. Playwright answered/api/enterprise/client-portal/askswith 200 (2 asks), then 503asks_unavailable.asksFailed=trueorigin/devstore)Portal.vuepage and the real backend round trip. The route-level 503 is covered bytest_ent610_asks_unavailable.pythroughTestClient.verify-local (python3.13,
--skip-agentbecause this diff doesn't touchdocker/base-image):Unit island:
test_ent477_definitions_endpoint(5),test_ent477_git_refresh_hook(10),test_ent666_objective_join(21),test_retention_floor(4).-p no:randomly, same venv) on both trees:origin/dev@ebc2bfb4fcommshows zero difference). The +9 passes are this PR's new tests.🤖 Generated with Claude Code