fix(mcp): unify worker-count across workers_list/system_stats/workspace_chat (#2270)#2283
Merged
Conversation
… of truth for the operator worker set (#2270) No behavior change: GET /workers (grid + MCP workers_list) now calls the new canonical function, which is its previous _list_visible_workers + system/archived filter moved behind one documented API that also returns the hidden count. Co-Authored-By: Claude Fable 5 <[email protected]>
…rker set (#2270) system_stats (MCP) proxies GET /stats, which counted a bare owner-scoped repos.workers.list(user_id) read: it included hidden internal/system workers and missed on-disk stock, workspace-shared and granted workers, so total_workers said 8 while workers_list said 13. Now routes through list_operator_workers (same set as the grid) and exposes the excluded system/archived count as WorkspaceStats.hidden_system_workers. Co-Authored-By: Claude Fable 5 <[email protected]>
…r set (#2270) Emily read repos.workers.list_for_agent (DB rows only, broader visibility tiers, its own stock/system hiding): she MISSED the on-disk stock workers the grid shows (csv_enricher, github-digest, outbound-approval-demo, research_brief) while SURFACING public/shared rows the grid hides (dogfood-test-url-grouper) — so she told the user a wrong worker count. Now routes through list_operator_workers with the grid role (auth context in requests, users-table fallback in-process; missing row degrades to member, never admin). include_system reveals the footnoted system/archived set; include_all_users is a schema-compat no-op. Tests that asserted the old narrower-than-grid Emily scope are updated to grid parity; out-of-scope cross-tenant seeds are no longer over-fetched-and-footnoted, they are simply out of scope. Co-Authored-By: Claude Fable 5 <[email protected]>
Full-app fixture mixing plain workers, a draft-stage worker, a manifest system_worker, an archived worker, and an internal engine worker id; asserts GET /workers, GET /stats total_workers and Emily workers__list_all agree on count, ids and hidden accounting, including under include_system. Co-Authored-By: Claude Fable 5 <[email protected]>
Member
Author
|
Independent adversarial codex review: CHANGES REQUESTED (CI is otherwise green; the one transient Windows failure — Findings from codex (gpt-5.6-sol), reviewing the diff against
Holding this PR pending fixes for the High/Medium items above. Not merging per delegated review process (codex flags = comment, not merge). |
Codex CHANGES-REQUESTED on PR #2283: - HIGH: get_workspace_stats built the worker set with _worker_access_user_id(auth) but queried stats_batch and runs.list with the raw auth.user_id. Under legacy/header-alias local-deploy identity mapping the two ids diverge, so stats_batch (owner_id-scoped) returned zero/incomplete active_workers, total_runs_7d and success_rate even though total_workers was correct. Resolve the access user id once and use it everywhere in the function. - MEDIUM: for role=admin, list_operator_workers returns an admin-wide worker set (other owners included) while stats_batch/runs.list stayed scoped to the caller-owned subset, so total_workers described a wider population than every other metric. Added an explicit unscoped mode (stats_batch(scope_to_owner=False), runs.list(worker_ids=...)) that trusts the already role-authorized worker_ids from list_operator_workers, so every workspace-stats field now describes the same population. All other callers keep the existing owner-scoped default. Added apps/api/tests/test_2283_stats_identity_alias.py covering both: a resolved-identity fixture and an admin fixture with another owner's worker, asserting active_workers/total_runs_7d/success_rate/avg_duration match the seeded population. Co-Authored-By: Claude Fable 5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2270.
Root cause
Three user-facing surfaces computed "the workspace's workers" from three different queries with three different filters:
workers_list(MCP) →list_workers→ GET /workers_list_visible_workers(apps/api/services/worker_access.py:650): role-scopedrepos.workers.list+ on-disk stock workers + owner-scoped FS fallback + grants, minus_worker_hidden_from_apisystem_worker, minus archived (inline inapps/api/routers/worker_listing.py:204)system_stats(MCP) → GET /statsrepos.workers.list(user_id=auth.user_id)— no role, owner-only (apps/api/routers/worker_telemetry.py:131)workers__list_all(workspace_chat)repos.workers.list_for_agent— DB rows only, broader visibility tiers (workspace/shared/public+ stock-id padding) (apps/api/services/chat_worker_tools.py:17)_worker_hidden_from_api+ manifest system + "unowned stock", footnoted ashidden_system_countConcretely: the 4 workers Emily was missing (
csv_enricher,github-digest,outbound-approval-demo,research_brief) are exactlyPUBLIC_STOCK_WORKER_IDSmembers that the grid surfaces from the on-disk stock path, whichlist_for_agent(DB-only) never sees — whiledogfood-test-url-groupercame in throughlist_for_agent's widerpublic/sharedvisibility clause. GET /stats missed both of those sources and failed to exclude internal engine workers, landing on a third number.Reproduced live pre-fix via MCP:
workers_list→ 13 workers,system_stats.total_workers→ 8.Fix
One canonical function —
services.worker_access.list_operator_workers(user_id, repos, role, include_system, include_archived)— with documented inclusion rules:_list_visible_workers(role-scoped DB workers + OSS on-disk stock + owner-scoped FS fallback + grants; engine-internal ids are never in the base set and never footnoted).system_worker: trueexcluded unlessinclude_system.include_archived. Draft/stage labels are cosmetic — drafts count like any worker.Returns
(visible_workers, hidden_count). All three surfaces now route through it:workers_list): unchanged behavior, now the canonical caller.system_stats):total_workers= canonical count with the caller's grid identity/role (_worker_access_user_id+_worker_repo_role); newWorkspaceStats.hidden_system_workersexposes the excluded system/archived count so "total vs user-visible" is explicit.workers__list_all: canonical set with the grid role (auth context in requests; users-table fallback for in-process calls, degrading tomember— never admin — when the row is missing).hidden_system_countkeeps footnoting the excluded set;include_systemopts it back in;include_all_usersis retained as a schema-compat no-op.list_for_agentremains in use for Emily's run-resolution path (_list_viewable_workers/_worker_can_view), which is a can-run gate, not a user-facing count — by-id runnability of stock workers is unchanged (covered by tests).Tests that asserted the old narrower-than-grid Emily scope were updated to grid parity (they encoded the split-brain this issue eliminates): admin default now equals the admin's grid view; out-of-scope cross-tenant seeds are simply out of scope instead of over-fetched-and-footnoted.
Test evidence
New
apps/api/tests/test_2270_worker_count_consistency.py: full-app fixture mixing plain workers, a draft-stage worker, a manifestsystem_worker, an archived worker, and an internal engine worker id (workspace-agent); asserts GET /workers, GET /statstotal_workersand Emily's tool agree on count, ids and hidden accounting, including underinclude_system.Targeted subset covering all three changed surfaces + the new fixture (
test_2270_worker_count_consistency,test_emily_worker_list_system_filter,test_emily_worker_split_brain_round09,test_emily_worker_legacy_scope,test_1139_emily_workers_admin,test_identity_ownership_and_emily_visibility,test_worker_tool_guards,test_monitoring_apis):The full
apps/apisuite runs in CI on this PR.Note on #2264
Confirmed distinct: #2264's "workers_list shape/limit passthrough" concerns response shape/param plumbing; this PR is the count-reconciliation across data sources.
🤖 Generated with Claude Code
Co-Authored-By: Claude Fable 5 [email protected]