fix: unify /agents card and stop card-click JSON dumps (#476, #482)#483
Conversation
Issue #476: typed `/agents` returned a Lark card mixing per-agent markdown blocks with a long row of "Status: …" buttons; users read it as a text list plus a second status panel. Replace the multi-card / multi-button layout with one consolidated card whose body is a structured agent list and whose footer is four global discovery / creation buttons. Per-agent operations stay accessible through the slash commands documented inline. The card-flow "Refresh List" path now reuses the same shared renderer so card-click and typed flows render identically. Issue #482: every `AgentBuilderCardFlow` formatter except daily-report used `BuildInfoCard` to serialize a Lark card JSON envelope and wrapped it in `MessageContent.Text`. The relay forwarded that JSON as plain text and clicking Templates / Status / Run / Disable / Enable / Confirm Delete or submitting Create Social Media surfaced raw `{"config":...}` blobs. Convert every card-click formatter (and the delete-confirmation card) to return `MessageContent` with structured `Cards` + `Actions` so the Lark composer renders native cards. Delete the now-dead Lark-JSON helpers (`BuildInfoCard`, `BuildButton`, `BuildLinkButton`, `EscapeMarkdown`, `BuildStatusCardActions`, `AgentListCardItem`, `ReadAgentList`). Also fix `LarkMessageComposer` non-form mode duplicating the first card title — it was already consumed by the card header but re-emitted as `**Title**` in the body. Form mode already skipped it; bring non-form mode to the same contract so single-card responses stop showing a redundant bold title row. Tests: add coverage for the new structured-MessageContent contracts on list_agents, list_templates, agent_status, run_agent, create_social_media, delete_agent ack, and the confirm-delete card-action; pin the no-duplicate header/body title in `LarkMessageComposerTests`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## dev #483 +/- ##
=======================================
Coverage 71.39% 71.39%
=======================================
Files 1219 1219
Lines 88382 88382
Branches 11565 11565
=======================================
+ Hits 63101 63104 +3
+ Misses 20748 20746 -2
+ Partials 4533 4532 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Review SummaryOverall this is a solid PR that cleanly fixes both #476 and #482 by replacing raw Lark JSON card blobs with structured I found a few issues worth addressing: 1.
|
eanzhao
left a comment
There was a problem hiding this comment.
Found one issue that needs fixing before merge:
agents/Aevatar.GAgents.Authoring.Lark/AgentBuilderCardContent.cs:278 now advertises /delete-agent <id> confirm from the shared /agents renderer. That renderer is also used by the direct AgentBuilderCardFlow text path, but TryParseAgentCommand in AgentBuilderCardFlow treats the full suffix after /delete-agent as the agent id. So if a direct webhook user follows the card and types /delete-agent skill-runner-1 confirm, the confirmation card is built for agent id skill-runner-1 confirm; clicking Confirm Delete then sends that bogus id and the delete fails. Either strip/handle the optional confirm token in the direct parser, or make the hint invoke the direct confirmation flow without appending confirm.
eanzhao
left a comment
There was a problem hiding this comment.
Review Summary
This PR cleanly fixes both #476 (/agents mixed-format) and #482 (card-click JSON dumps). The approach of consolidating the /agents renderer into a shared AgentBuilderCardContent.FormatListAgentsResult is sound, and converting the card-click formatters from raw Lark JSON strings to structured MessageContent resolves the root cause of both issues.
Build & tests: Green across all affected projects.
Minor observations (non-blocking)
-
Dead code in NyxRelayAgentBuilderFlow.cs:340 —
FormatAgentStatusResult(the string-returning version) is no longer referenced; the dispatch at line 70 now goes throughFormatAgentStatusCard. This was likely dead before this PR too, but worth a cleanup pass. -
Duplicated helpers —
BuildActioninAgentBuilderCardContent(line 305) andBuildCardActioninAgentBuilderCardFlow(line 906) are identical implementations. Similarly the agent-scoped variants. Consider consolidating intoAgentBuilderCardContentas the shared builder. -
UX inconsistency in status card buttons —
NyxRelayAgentBuilderFlow.FormatAgentStatusCardshows "Disable" and "Enable" buttons simultaneously regardless of the agent's actual status, whileAgentBuilderCardFlow.FormatAgentStatusResultshows them conditionally (one or the other). This means users see different button layouts depending on whether they typed/agent-statusor clicked through from the/agentscard. Pre-existing, but worth aligning.
Blocking: `/delete-agent <id> confirm` direct text path used to be parsed by `TryParseAgentCommand` which takes the entire suffix as the agent_id, so following the new shared `/agents` renderer's inline hint `/delete-agent <id> confirm` produced an agent_id of `<id> confirm`. The confirmation card was then built for that bogus id and the subsequent Confirm Delete click failed. Replace the delete branch with a tokenizing parser that recognizes the optional `confirm` trailer and dispatches the delete directly when present (matching NyxRelay's text contract); when absent, the existing UI-driven confirmation card path is unchanged. Maintenance: extract duplicated `agent_builder_action` identifiers into a shared `AgentBuilderActionIds` constants class, eliminating the silent- divergence hazard between `AgentBuilderCardContent` and `AgentBuilderCardFlow`. Extract the triplicated JSON readers (`TryReadError`, `TryReadString`, `TryReadBool`, `TryReadOptional`, `TextContent`) into `AgentBuilderJson`, with the per-class wrappers kept as one-liners that delegate so the formatter sites stay readable. Cleanup: delete the unreachable string-returning `NyxRelayAgentBuilderFlow.FormatAgentStatusResult` left over from before `agent_status` switched to the structured-card path. Docs: clarify the `LarkMessageComposer` non-form `skipTitle = i == 0` comment with the precedence invariant from `ResolveHeaderTitle`, and note that the skip is a no-op when the first card has no title. Tests: pin `/delete-agent <id> confirm` and `/delete-agent <id>` direct text paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fix Review: All 5 Items Resolved ✅1. Constants duplication →
|
Summary
Fixes #476 and #482, which are two facets of the same underlying problem in the Lark agent builder flow.
Issue #476 —
/agentsmixed text list + status cardTyped
/agentspreviously returned aMessageContentwith N+1CardBlocks (1 summary + N per-agent rows) and N+3 action buttons (N "Status: …" + 3 footer). When the Lark composer compiled this, the user saw stacked markdown blocks (perceived as a text list) followed by a long button row whose "Status: …" labels read as a separate status panel — exactly the dual-format experience the issue calls out.The new design produces one consolidated card:
Your Agents (N), body lists every agent in a structured markdown block (template · status, agent ID, next run, optional last run)./agent-status <id>,/run-agent <id>,/disable-agent <id>·/enable-agent <id>,/delete-agent <id> confirm) — keeps the Scheme A "pure structured list" feel from the issue while preserving discoverability./agents(NyxRelayAgentBuilderFlow) and card-click "Refresh List" (AgentBuilderCardFlow), and is also reused as the post-delete acknowledgment with the delete notice prepended.Issue #482 — card buttons returned raw JSON
Every
AgentBuilderCardFlowformatter except daily-report usedBuildInfoCardto serialize a Lark card JSON envelope and wrapped that string inMessageContent.Text. The relay forwarded the JSON as plain text, so clicking Templates, Status, Run, Disable, Enable, Confirm Delete, or submitting Create Social Media surfaced raw{"config":...}blobs.Every card-click formatter (and the delete-confirmation card) now returns proper
MessageContentwith structuredCards+Actionsso the Lark composer renders native cards. The legacy Lark-JSON helpers (BuildInfoCard,BuildButton,BuildLinkButton,EscapeMarkdown,BuildStatusCardActions,AgentListCardItem,ReadAgentList) are deleted.Bonus: composer header/body title duplication
LarkMessageComposernon-form mode emitted the first card'sTitletwice — once as the card header and again as**Title**at the top of the body markdown. Form mode already skipped the first title; bring non-form mode to the same contract. Every single-card response (the new/agents,/agent-status, etc.) stops showing a redundant bold title row.Affected paths
agents/Aevatar.GAgents.Authoring.Lark/AgentBuilderCardContent.cs— new sharedFormatListAgentsResult(JsonElement, string? notice)renderer.agents/Aevatar.GAgents.Authoring.Lark/NyxRelayAgentBuilderFlow.cs—list_agentsdelegates to the shared renderer; deadFormatListAgentsCard/FormatListAgentsResult(text) /ShortenAgentIdremoved.agents/Aevatar.GAgents.Authoring.Lark/AgentBuilderCardFlow.cs— every card-click formatter rewritten to returnMessageContent; delete-confirmation card returnsMessageContent; legacy Lark-JSON helpers and unused records deleted.agents/platforms/Aevatar.GAgents.Platform.Lark/LarkMessageComposer.cs— non-form mode skips the first card's title in body markdown (consistent with form mode); empty markdown blocks are dropped.test/Aevatar.GAgents.ChannelRuntime.Tests/{AgentBuilderCardFlowTests,NyxRelayAgentBuilderFlowTests}.cs— updated/added coverage for the new contracts.test/Aevatar.GAgents.Platform.Lark.Tests/LarkMessageComposerTests.cs— pins the no-duplicate header/body title invariant.Verification
dotnet build agents/Aevatar.GAgents.Authoring.Lark/Aevatar.GAgents.Authoring.Lark.csproj— 0 errors, 0 new warnings.dotnet test test/Aevatar.GAgents.ChannelRuntime.Tests/Aevatar.GAgents.ChannelRuntime.Tests.csproj— 565 passed.dotnet test test/Aevatar.GAgents.Platform.Lark.Tests/Aevatar.GAgents.Platform.Lark.Tests.csproj— 15 passed.bash tools/ci/architecture_guards.sh— all guards passed up to the playground asset drift guard, which fails identically on the base branch (pre-existing infra; unrelated).bash tools/ci/test_stability_guards.sh— passed.Test plan
/agentsand confirm a single card with structured agent rows + four footer buttons (no per-agent button row, no duplicated bold "Your Agents" title)./agent-status <id>(typed) and from the Refresh-List → Status flow, confirm Run / Disable / Enable / Delete buttons render natively and submit cleanly./agentscard with a "Deleted agent …" notice on top.🤖 Generated with Claude Code