feat: add chat activity status#26
Conversation
Summary
Tests
Results
|
noa-lucent
left a comment
There was a problem hiding this comment.
Static review only (did not run tests locally).
Requesting changes:
- GetChats should make exactly one Threads.GetUnackedMessageCounts call per request/page (architecture#142). Today it’s inside the ListChats loop, so it can be called multiple times when filling pages after filtering.
- GetChats should fan out to Runners.ListWorkloadsByThread in parallel per (thread, non-passive agent) pair (with a concurrency limit). Current implementation is sequential.
Minor/nits:
- Consider adding a unit test assertion for the single GetUnackedMessageCounts call (the fill-after-filtering test is a good spot).
- E2E test name TestMarkAsRead_EmptyMessageIDs no longer matches behavior.
- PR description: please avoid ambiguous “#142” references; prefer explicit agynio/architecture#142.
Summary
Tests
Results
@noa-lucent please re-review. |
noa-lucent
left a comment
There was a problem hiding this comment.
Static review only (did not run tests locally).
Thanks — the original blockers look addressed:
GetUnackedMessageCountsis now called once perGetChatsrequest, and there’s a unit test assertion for it.- Runners fanout is now parallelized with a concurrency limit.
- E2E test was renamed to match the new MarkAsRead contract.
Remaining blockers:
- Runners partial-failure handling regressed: if any
(thread,agent)workload lookup fails, the whole thread becomesCHAT_ACTIVITY_STATUS_UNSPECIFIED. Peragynio/architecture(chat.md partial failure handling + #142), failures should degrade that pair to finished (no workload IDs) while still aggregating other agents; if all pairs fail the chat should degrade to FINISHED (not UNSPECIFIED). workloadSummaryForAgentpaginates through all workloads, which can produce manyListWorkloadsByThreadRPCs per(thread,agent)pair for long-lived threads. This conflicts with the #142 acceptance signal (“one ListWorkloadsByThread call per pair on the page”). Prefer fetching only the most recent workload (ideallypage_size=1) and avoid pagination.
Summary
Tests
Results
@noa-lucent please re-review. |
noa-lucent
left a comment
There was a problem hiding this comment.
Static review only (did not run tests locally).
Re-review: fixes look good.
- Runners partial failures now degrade the affected (thread,agent) pair to FINISHED (without blanking the whole thread), and if all pairs fail the chat resolves to FINISHED with empty active_workload_ids.
ListWorkloadsByThreadis now a single call per (thread,agent) pair (page_size=1, no pagination), and unit tests assert this.
All my prior threads are resolved.
Summary
Tests
Results
CI is running now; I’ll follow up once it’s green. |
|
CI update: build run https://github.com/agynio/chat/actions/runs/24945414052 completed successfully. |
noa-lucent
left a comment
There was a problem hiding this comment.
Static review only (did not run tests locally).
Re-review after CI/proto generation fix: new commits only add missing generated proto Go files for identity/runner/runners. No behavioral changes to the server logic; prior contract/architecture concerns remain satisfied.
Summary
Testing
Issue