fix(workspace): a client no longer sees loop runs it cannot open or explain (#2423)

[dolho]

Journey Impact: none: narrows what one existing surface reports to one principal kind — no user-facing promise is added or extended.

Closes #2423

The problem

The Workspace told a client its agent had run 12 loops — a Loops 12 legend entry and a run of rows saying Loop with per-run durations — and gave it nowhere to go.

Verified against a real client session, not inferred:

client-visible by_type: {'Public': 4, 'Loops': 12, 'Chat/Tasks': 1}
recent_work           : 17 rows, mostly triggered_by='loop'

Four reasons there was nowhere to follow up:

• the loops strip is isPlatformSession-gated (ent#458, correctly — loops are an operator capability)
• even for an operator that strip shows status, run count and Stop — never output
• the Workspace agent page has no Loops tab
• per-run results live only on operator Agent Detail → Loops and Operations → Executions

So the loop count was client-visible while the loop output was operator-only.

The direction is not a new product call

The issue left show it vs hide it open on purpose. agent_page's own docstring already answers it:

It reports; it does not configure. … The viewer may be an external client, not an operator.

The module is subtractive by design — it projects away message, cost, model_used, source_user_email, and already drops alert asks as "operations telemetry, not something the agent is asking a person." A loop run is the same kind of thing. This follows that rule rather than inventing a second one.

But not subtractive for everyone

The same page serves a platform user, who can click through to Agent Detail → Loops and read every run — so hiding it from them removes real signal and fixes nothing.

Split by principal, using the is_platform the route already resolves for get_agent_card one line above. Same pattern as the roster and _require_roster. The client view is the default, so a caller that forgets to say who is looking gets the projection that leaks least.

Both halves or neither

Rows and chart are filtered together. Removing the rows and leaving Loops 12 in the legend would be worse than doing nothing — a number with nothing behind it.

Day totals and the headline are re-derived: a bar labelled 13 whose segments sum to 1 reports its own filtering as missing data.

success_rate is deliberately not recomputed — it's a ratio over terminal rows this function cannot see, and a filtered numerator over an unfiltered denominator would be worse than a figure that is merely broad.

A trap worth recording

The two by_type fields have different shapes under one name:

"by_type": by_type_totals,   # top level: LIST of {"bucket", "total"}
"by_type": by_type,          # per day:   DICT  {bucket: count}

My first draft handled only the dict and crashed the whole page on a real payload — and my own test fixture had the same wrong shape, so it passed against something the accessor never emits. test_2161_agent_page_ux::test_stats_forward_the_canonical_bucket_order caught it.

Both are corrected. The helper now tolerates either form, and an unrecognised row is kept — silently hiding a row we failed to parse would be the opposite of this function's job.

Verification

pytest -k "portal or 2423 or 2160 or 2161 or 2169 or ent360"   → 410 passed
tests/unit/test_2423_client_loop_visibility.py                 → 11 passed

Mutation-checked — each turns the suite red on its own:

Mutation	Result
Drop the row filter	2 failed
Leave day totals stale	1 failed
Leave the headline stale	1 failed

Not from this branch

test_ent457_portal_turn_kwargs and test_both_portal_row_creation_sites_name_the_chat fail on dev today — backend-unit-test is red there. Both are repaired in #2427; this branch neither causes nor fixes them.

[obasilakis]

One blocker, plus a process item the issue itself asked for.

Blocker

src/backend/client_portal/agent_page.py:334-340 — the filter is applied after the SQL LIMIT, so the client list starves.

db.get_agent_executions_summary(agent_name, limit=20) limits in SQL (executions.py:558-559, .order_by(started_at desc).limit(limit)), and loop rows are then dropped in Python. On this PR's head, an agent whose 20 newest rows are all loops yields zero rows from _recent_work(is_platform=False), and PortalAgentPage.vue:174/346 renders "Nothing yet." / "No activity in this window." while the operator sees 20. That is precisely the loop-heavy agent ent#458 describes — its own repro is "17 rows, mostly loops", which already leaves roughly five survivors. It trades "12 rows I can't explain" for a false claim of no activity. Over-fetch (limit * N, or effectively unbounded) and slice to limit after filtering.

Comments

• The product call the issue asked for was never recorded. #2423 says the direction "shouldn't be settled by whoever picks the issue up — either answer is defensible" and lists two contradicting options; the issue has zero comments, and this PR picks the hide direction and argues in the body that it isn't a new call. That flag is the author's own note on his own issue, self-answered with a documented rationale (the agent_page docstring's subtractive rule), so nobody's stated plan is being overruled — but it still wants a second party's sign-off before merge, since that is what the issue asked for.
• _recent_work post-filters while neither success_rate nor first_try is recomputed. The body documents the success_rate decision explicitly but is silent on first_try: client_portal/db.py:775-783 counts all terminal rows, so a client's first-try rate keeps a loop-inclusive denominator. Consistent with the stated choice, just undocumented.
• Issue #2423 carries status-in-dev while it is open and this PR is unmerged (set at PR-open). Should be status-in-progress; the merge automation sets status-in-dev.
• Head is three commits behind origin/dev (#2422, #2425/#2424). No file overlap and still MERGEABLE.

Docs

docs/memory/feature-flows/workspace-agent-page.md is not updated, and this PR's subject is that file's subject:

• :26-38, the "Exclusion by projection, not by template" table enumerating recent_work exclusions, does not mention the new principal-split loop exclusion.
• :217 and :531 are now factually false for a client — "a chat, loop or reminder row … keep trigger, duration and time", when a loop row no longer reaches a client at all.
• :228 already anticipated this ("Gating it on principal.is_platform is one line away"), so the flow predicted the change and wasn't amended when it landed.

Also: docs/memory/requirements/core-agent.md:478's projection list is now incomplete, and there's no docs/memory/feature-flows.md recent-changes row (cf. the #2258 and #2424 rows). architecture.md is correctly untouched — no endpoint or schema change.

What's clean

Security greps all clean, base dev, three files, one conventional commit, Closes #2423 resolves same-repo so auto-promotion will fire, CI 20/20.

Tests are strong and I mutation-checked them myself: dropping the row filter turns two red, stale day totals one, stale headline one, and a fourth mutation I added (default flipped to is_platform=True) turns one red, so the fail-closed default is pinned. _without_hidden_buckets is sound — _BUCKET_ORDER (db/schedules/analytics.py:62-68) has an Other catch-all, so every execution lands in a bucket and sum(kept_totals) == total_executions; both by_type shapes are handled and hasChartActivity/bucketsForChart degrade correctly.

The pre-existing-failure claim also checks out: test_ent457_portal_turn_kwargs and test_ent457_portal_completion_report fail on origin/dev itself in a clean worktree, so this PR neither causes nor fixes them.

[dolho]

Addressed — eef247cc, rebased onto dev

The blocker is real, and worse than the bug I fixed

get_agent_executions_summary limits in SQL; my filter ran in Python. An agent whose newest 20 rows are all loops yielded zero rows and rendered "Nothing yet." while the operator saw twenty — trading rows I can't explain for this agent has done nothing. The first is confusing; the second is false.

And you're right that it isn't an edge: ent#458's own repro ("17 rows, mostly loops") already leaves ~5 survivors. It's the normal shape for exactly the agents this feature exists for.

Fixed by over-fetching on the side that filters and slicing back:

fetch = limit if is_platform else limit * _CLIENT_OVERFETCH   # 5x
...
rows = [r for r in rows if r.get("triggered_by") not in _CLIENT_HIDDEN_TRIGGERS][:limit]

Three properties I'd call out: the operator side does not over-fetch (nothing is filtered there, so it'd be pure waste); the cap moves rather than disappears, so one client's page can't cost a whole agent history; and past the multiplier the list is genuinely shorter — which is honest, since that is what the agent has been doing, and the chart beside it carries the same window.

Three tests, mutation-checked: reverting to limit-then-filter turns two red, over-fetching without the slice turns the bound red.

first_try

Correct — same reasoning as success_rate, and I documented only one of them. Now stated in both the flow doc and the changelog row: a filtered numerator over an unfiltered denominator is worse than a figure that's merely broad.

Docs

All the sites you named:

• the exclusion table gains the principal-split row and the LIMIT-interaction note
• :217's "chat, loop, reminder" no longer claims loop rows reach a client
• :228's "gating it on principal.is_platform is one line away" now records that Workspace shows a client 12 loop runs it cannot open, control, or explain #2423 took exactly that route — you were right that the flow predicted this and wasn't amended
• requirements/core-agent.md:478 projection list
• feature-flows.md recent-changes row, in the fix(workspace): Sign out doesn't sign out — the platform JWT silently re-authenticates on refresh #2258/Workspace says 2 asks are waiting and gives you no way to find them #2424 shape

Label

Fixed → status-in-progress. That was my error at PR-open.

Rebase

Done — and #2427 merged in the meantime, so the two pre-existing failures are gone. This branch is now 428 passed, 0 failed on the portal + ent#457 selection.

---

The product call — you're right to hold it

The issue asked for a second party's sign-off and I self-answered. My reasoning is in the PR body: agent_page's own docstring already decides it ("it reports; it does not configure … the viewer may be an external client"), and it already drops alert asks as "operations telemetry, not something the agent is asking a person" — so hiding follows the existing rule where showing would need a new one. The operator split is what keeps it from being a pure subtraction.

But that's me agreeing with me. @obasilakis — does the hide direction sit right with you, or would you rather loop output reached clients through deliverables (ent#365/#425)? Happy to hold the merge on that.

Thank you for mutation-checking the tests independently, and for the extra is_platform=True-default mutation — that's a better pin than the one I wrote.

[obasilakis]

Re-validated on eef247cc. Nine of the eleven items from the last pass are cleanly fixed — the flow's exclusion table and its :217/:228 claims, the requirements projection list, the index changelog row, the first_try denominator note, the issue's status-in-progress label, and the rebase (0 behind dev). The over-fetch itself is correctly implemented and properly pinned: reverting fetch = limit turns test_a_loop_heavy_agent_still_shows_its_other_work red, and dropping the [:limit] slice turns test_the_client_list_is_still_bounded red.

Direction 2 has my sign-off. Thanks for recording the call on the issue with the case against it — that is what #2423 asked for. The subtractive rule is this module's own, and direction 1 is genuinely a separate feature rather than the other half of this one.

Two things before it merges.

1. agent_page.py:354,366 — the starvation is narrowed, not closed, and the boundary is exact

fetch = limit if is_platform else limit * _CLIENT_OVERFETCH with MAX_RECENT_WORK = 20 and _CLIENT_OVERFETCH = 5 moves the threshold from 20 consecutive loop rows to 100. Above 100 the client list is not shorter, it is empty, and PortalAgentPage.vue:174/346 renders "Nothing yet." / "No activity in this window." again. Verified on head with a 150-loop / 5-chat fixture: client 0 rows, operator 20.

The reason I do not think a larger multiplier is the answer: models.py:2768 sets MAX_RUNS_LIMIT = 100, so a single loop at the documented maximum produces exactly 100 rows and exactly exhausts the over-fetch window. The constant lands precisely on the ceiling the loop feature ships with, and ent#458 rooms multiply that by participant. These are the agents the fix exists for, not a contrived depth.

The comment at :346-352 calls the outcome "honest — it is genuinely what that agent has been doing". That reasoning is right for a truncated list and does not carry to an empty one, which asserts something false.

Pushing the predicate into SQL — an exclude_triggers argument on get_agent_executions_summary, i.e. WHERE triggered_by NOT IN (:x) — closes this and the read amplification below in one move. Failing that, fetch iteratively until limit survivors or exhaustion.

Related, and the same change fixes it: the client path now pulls 100 rows of a ~21-column select that includes message, the full prompt text (db/schedules/executions.py:541), and portal_agent_page (router.py:590) carries no rate limit, unlike the sibling report-rows route. The comment at :341 says "it is a cap on the READ, so the cost is bounded either way" — bounded, but five times the bytes.

2. workspace-agent-page.md:544 — the Known Limitations line I cited by number is still there

Byte-identical to dev:

A chat, loop or reminder row has no equivalent safe label … so those rows keep trigger, duration and time.

For a client a loop row no longer reaches the page at all. This is the table a reader consults last and trusts most.

Three further sites the flow's own structure requires:

• :472-536 Tests — enumerates each test file and what it pins (test_ent360_* at :474, test_2161_* at :486, test_2162_* at :501). tests/unit/test_2423_client_loop_visibility.py is absent.
• :446-471 Files — carries a per-issue row for #2161 and #2162. There is no #2423 row for agent_page.py's new _without_hidden_buckets / _bucket_of / _total_of and two constants, nor for router.py.
• :97-111 The two AC #3 metrics — the section that exists to explain success_rate against first_try is where the now-divergent denominator belongs. Recording it only in the index changelog row puts it where nobody reading those metrics will look.

Worth deciding, not blocking: the strip now contradicts itself

_stats re-derives total_executions (agent_page.py:142) but leaves success_rate (:216) and first_try (:228) over the unfiltered set, and PortalAgentPage.vue:50/54/58 renders all three side by side. On a loops-only agent that reads:

0 tasks · 100% completed · 100% first try

Before this PR it read 150 / 100% / 100% — wrong in a different way, but at least internally consistent. This is the PR's own objection ("a number with nothing behind it") re-created one tile over, now as percentages.

Filtered numerator over unfiltered denominator is correctly rejected in the body. The third option was not considered: render — for both rates when the client-visible count is 0. pct() already renders — for null (PortalAgentPage.vue:578), so it is a null rather than a UI change.

Same class one field further along: agent_page.py:493 _last_active reads the newest execution unfiltered, so a loops-only agent shows a client "Last active 2 minutes ago" above "Nothing yet."

Smaller note

agent_page.py:146-158 _bucket_of — for the dict-shaped by_type it says it tolerates, the iteration walks keys, _total_of returns 0 for each, and the result is total_executions: 0 above a chart summing 5. That is precisely the headline-disagrees-with-the-chart inconsistency _without_hidden_buckets exists to prevent, arriving through the branch meant to be defensive. The accessor emits the list shape today (db/schedules/analytics.py:642) so it is latent — either handle the dict explicitly or drop the tolerance and let the existing #2161 test pin the shape.

What is clean

The filter is in the service (agent_page.py:366, :212) rather than in Vue, which is this module's stated principle; it is keyed on principal.is_platform, the same flag _require_roster and get_agent_card already use; the default is is_platform=False and is pinned by test_the_client_view_is_the_default; hiding produces absence rather than a distinguishable 403, so Invariant #8 is intact; _schedule_names receives already-filtered rows so no schedule-id lookups leak for hidden rows; and there is a single call site, with no portal loops endpoint to become a second surface. Security greps all clean, Closes #2423 resolves same-repo so promotion will fire, CI 20/20.

[dolho]

/review — fresh pass on a4064a23

Branch: fix/2423-client-loop-visibility → dev · +865/-34 across 9 files
Scope: CLEAN — service, its facade, one db accessor, the router flag, two test files, three docs
Base: merge-base 135248e9

Re-reviewing my own fix commit. Both pass-2 blockers are closed, and I checked the load-bearing one by mutation rather than by reading:

• The filter is in SQL. get_agent_executions_summary(..., exclude_triggers=) adds a WHERE, so the LIMIT applies to rows that already survived it. Moving the filter back after the limit makes test_one_max_length_loop_does_not_hide_everything_else fail — verified, not assumed. Starvation is gone at any run length, and the extra read went with it (client and operator both fetch exactly MAX_RECENT_WORK).
• The docs. Seven sites, including four repeating a rating-tally claim false since ent#366 — one of them build_page's own docstring, twelve lines from _rating_tally.

One new finding, fixed in the same push, plus one minor.

---

I1 → fixed: the two halves of one decision had nothing connecting them (Confidence 9/10)

_CLIENT_HIDDEN_TRIGGERS = frozenset({"loop"})    # a triggered_by value
_CLIENT_HIDDEN_BUCKETS  = frozenset({"Loops"})   # a display label

The comment beside them says a single constant "would hide that a rename on either side breaks the pair" — correct, and then it left nothing to notice the break. "Loops" is produced by db/schedules/analytics.py::_TRIGGER_BUCKETS["loop"], and I grepped: no test relates the two.

Rename that label to "Agent loops" and this PR half-reverts in silence — the chart shows loop counts to clients again while _recent_work still hides the rows, which is exactly the "legend reads 12 above a list that says nothing" contradiction the change exists to remove. Nothing anywhere fails.

Fixed with a guard derived from the real map rather than a second copy of the literal:

expected = {_TRIGGER_BUCKETS[t] for t in page._CLIENT_HIDDEN_TRIGGERS}
assert page._CLIENT_HIDDEN_BUCKETS == expected

It also asserts every hidden trigger is a mapped trigger, so hiding a row type with no corresponding bucket is caught too. Mutation-verified: renaming the label fails the guard; reverting passes.

I2 → fixed: a DB round-trip on the path that discards it (Confidence 8/10)

first_try_stats(agent_name, hours) was computed above the zero-suppression gate, and the withheld branch returns a hardcoded zero dict — so a client viewing a fully-hidden agent paid for a query whose result could not be used. Moved below the gate.

---

Clean, with what was checked

• Index — idx_executions_agent_started ON schedule_executions(agent_name, started_at DESC) (db/schema.py:1609) still drives the ordering; the new triggered_by predicate is a filter applied while walking it, so a loop-heavy agent reads more index entries but does not lose the index. This was the thing worth checking before moving a filter into SQL.
• .where() after .limit() — reads like a pipeline, isn't one: SQLAlchemy composes a statement, so the emitted SQL is WHERE … ORDER BY … LIMIT. Proven by test_one_max_length_loop_does_not_hide_everything_else against real SQLite, and by the mutation.
• NULL triggered_by — explicitly admitted via or_(is_(None), notin_(...)). NOT IN yields NULL for a NULL left side and the row would vanish; the column is NOT NULL, so this is defence, and it is tested.
• Every existing caller — routers/schedules.py:825 and _last_active pass no exclude_triggers; if exclude_triggers: treats None and frozenset() alike, so no WHERE is added and behaviour is byte-identical. Pinned by test_no_exclusion_is_the_unchanged_behaviour.
• Fail-closed default — is_platform=False on _stats, _recent_work, _last_active and build_page; a caller that forgets gets the projection that leaks least, and test_the_client_view_is_the_default pins it. router.py:615 is the only production call site.
• Auth — no gate changed. The split is a projection inside an already-roster-gated route, on the same principal.is_platform get_agent_card keys on.
• Withheld ≠ zeroed — the zero branch returns rate: None (UI em-dash), never 0, and unavailable: False because the read succeeded. Both asserted.
• Test stubs model SQL — every stub in the older file routes through one _sql_like helper that filters then limits. A stub doing it the other way round is the bug under test and would pass against the broken implementation; that is why the accessor gets its own real-SQLite file.

Summary

• Critical: 0
• Informational: 2 — both fixed in this push
• Scope: clean

[obasilakis]

Re-reviewed on c103eec7. Both blockers are closed, and the first one is closed better than I asked for.

1. The starvation is gone, not narrowed. exclude_triggers on get_agent_executions_summary puts the predicate in the WHERE, so the LIMIT applies to rows that already survived it — no multiplier to outgrow, and the client page drops back to fetching MAX_RECENT_WORK rows instead of 100 of a 21-column select carrying message. _last_active carries the same exclusion, which closes the "active 2 minutes ago" above "Nothing yet." I raised separately. Not excluding NULL triggered_by is the right call and the reason is correctly stated at the accessor: NOT IN yields NULL for a NULL left side, and an unclassified row is not a hidden one.

tests/unit/test_2423_executions_summary_exclude.py is the test this needed — a real SQLite through the real engine, because no Python stub can prove WHERE precedes LIMIT, and the 100-loop case is MAX_RUNS_LIMIT rather than a contrived fixture. The newest-surviving-rows and existing-callers-unchanged cases matter as much as the headline one.

2. Docs. All four sites landed — the Known Limitations row removed, the Tests section carrying both new files, the Files table carrying the #2423 rows, and the two-metrics section explaining the withholding. The rating-tally correction is a good catch that was not asked for; that paragraph had been false since ent#366.

3. The self-contradicting strip. Withholding both rates at exactly zero visible executions is the third option and the right one — pct() already renders null as an em-dash (PortalAgentPage.vue:595), so it is a null rather than a UI change, and moving first_try_stats below the gate saves the round-trip on the one case that discards it.

4. The drift guard. test_the_hidden_trigger_and_the_hidden_bucket_cannot_drift derives {_TRIGGER_BUCKETS[t] for t in _CLIENT_HIDDEN_TRIGGERS} from the real map, so it reds on a rename in either file rather than pinning a second copy of the literal. That is the guard the comment was asking for, and the learnings.md entry generalises it correctly.

_bucket_of's tolerance is now justified by what it is actually for — an unparseable row must still reach _total_of and be counted, so keeping it is not defensive padding.

One thing before it merges

docs/memory/feature-flows/workspace-agent-page.md — the prose block sits inside the "What must not ship" table, and it breaks it. Lines 47–51 on head:

NULL `triggered_by` is explicitly NOT excluded. ... an unclassified row is not a hidden one.
| `asks` | `context`, and `alert`-type items | ... |
| report detail | any report in the install | ... |

A pipe line immediately after a paragraph line is not a table — GFM needs a header plus delimiter row, and there is no blank line either — so the asks and report detail rows render as literal text with pipes in them. That is the table documenting the context credential-leak exclusion (canary G-04) and the report-id ownership check, i.e. the two entries a reader is most likely to consult. It predates this pass (it arrived with eef247cc) but it is this PR's, and the fix is to move the whole prose block below the last table row.

Non-blocking

• _stats' withheld branch returns "buckets": [] and "by_type": [] while the normal path returns a.get(...). Equivalent today because a is already filtered, but the two shapes are written differently, so a future change to _without_hidden_buckets only lands on one of them.
• The zero-gate fires on a genuinely empty window too (a brand-new agent), which is the same em-dash it already showed there. Worth a sentence in the flow doc so nobody later reads the branch as loops-specific.

Status

CI is still running on head — six pytest matrix jobs IN_PROGRESS as of this comment, everything else green. Approving on the doc fix plus a green matrix.

[obasilakis]

Approved.

Both blockers are closed — the exclusion is in SQL ahead of the LIMIT, _last_active carries it too, the rates are withheld rather than zeroed, and the trigger/bucket correspondence is derived from _TRIGGER_BUCKETS rather than restated. The real-SQLite accessor test is the right proof and the 100-row case is MAX_RUNS_LIMIT, not a contrived fixture.

Before merge, please push: docs/memory/feature-flows/workspace-agent-page.md — the prose block sits inside the "What must not ship" table, so the asks and report detail rows follow a paragraph line with no header and render as literal pipe text. Move the prose below the last table row. That table is where the context credential-leak exclusion and the report-id ownership check are documented.

Also confirm the pytest matrix goes green — six jobs were still IN_PROGRESS when I reviewed.

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.