fix(agent): suppress empty final content instead of sending literal "..."#1116
fix(agent): suppress empty final content instead of sending literal "..."#1116nguyennguyenit wants to merge 1 commit into
Conversation
…..." When the agent's final content is empty after sanitization, both v2 loop_finalize.go and v3 finalize_stage.go used to substitute the literal string "..." and ship it to channels. Users saw stray "..." messages in group chats (Zalo/Telegram/Discord/etc.) when the LLM produced no usable visible text (provider parsing quirk, reasoning-only output, etc.). Treat empty FinalContent as silent (same delivery path as NO_REPLY) and emit a structured slog.Warn with session/iteration/thinking_len/tool_calls so the underlying cause stays visible for debugging without leaking "..." to end users. NO_REPLY semantics unchanged. Downstream suppression in cmd/gateway_consumer_normal.go and gateway_announce_queue.go already handles empty Content correctly; the "..." fallback was bypassing that check. Adds regression tests for empty / NO_REPLY / real-content paths.
mrgoonie
left a comment
There was a problem hiding this comment.
Reviewed latest head 18a9eb7. This is a small, targeted fix for the user-visible ... leak: both v2 finalize and v3 pipeline now treat empty final content as silent delivery while preserving NO_REPLY semantics and adding warning logs for root-cause debugging.nnEvidence checked:n- Diff is limited to internal/agent/loop_finalize.go, internal/pipeline/finalize_stage.go, and internal/pipeline/stages_test.go (+113/-22).n- Current consumers already suppress empty content paths, so removing the placeholder aligns finalize output with downstream behavior.n- Duplicate search for empty-final/ellipsis/NO_REPLY found no overlapping open issue/PR.n- Regression coverage includes empty suppression, NO_REPLY suppression, and normal content pass-through.n- Local verification: go test ./internal/pipeline ./internal/agent passed.nnNo Critical or Important findings from this bounded review. Merge is still blocked by conflicts against current base, so this approval is for the implementation intent at the reviewed head, not a merge-ready signal until the branch is updated.
mrgoonie
left a comment
There was a problem hiding this comment.
Rechecked in this cron pass. Latest reviewed head remains approved in intent, but GitHub still reports mergeState DIRTY, so this is not merge-ready and I am not attempting merge. Please rebase/update the branch against the current base; after conflicts are resolved and CI reruns green, it can be merged.
|
Cron-safe recheck: latest head |
mrgoonie
left a comment
There was a problem hiding this comment.
Summary: This removes the user-visible literal ... fallback from both v2 and v3 finalize paths, treats empty final content as silent delivery, and keeps NO_REPLY semantics intact. The added pipeline regression coverage covers empty, NO_REPLY, and normal content.nnRisk level: Low/Medium. Behavior changes only when FinalContent is empty; downstream consumer paths already suppress empty content.nnMandatory gates:n- Duplicate / prior implementation: clear — search found this PR only for the empty FinalContent / ellipsis symptom.n- Project standards: docs found — Go backend, raw small focused changes, tests in the touched package.n- Strategic necessity: clear value — prevents confusing channel-visible ... messages while preserving logs for root-cause investigation.nnFindings: none blocking.nnVerification:n- Reviewed changed files: internal/agent/loop_finalize.go, internal/pipeline/finalize_stage.go, internal/pipeline/stages_test.go.n- Checked downstream delivery suppresses empty content in cmd/gateway_consumer_normal.go, cmd/gateway_announce_queue.go, cmd/gateway_subagent_announce_queue.go, and channel dispatch guards.n- Ran go test ./internal/pipeline/ ./internal/agent/ successfully.nnVerdict: Approve. Merge is still blocked until the PR is made mergeable/rebased because GitHub reports mergeStateStatus=DIRTY.
|
Cron-safe recheck: latest head |
|
Metadata refresh from github-maintain: I rechecked this PR and it is still valuable, but it remains non-mergeable because GitHub reports Review status:
Current blocker:
No approval/merge attempted in this run because the PR is dirty and not merge-ready. |
mrgoonie
left a comment
There was a problem hiding this comment.
Reviewed current diff against dev/main finalize paths. This is a narrow, useful fix: it removes the literal "..." empty-output fallback from both v2 and v3 finalization, preserves NO_REPLY suppression, and adds v3 regression coverage for empty / NO_REPLY / real-content behavior. Duplicate search only found this PR, and CI is green.nnNo blocking findings. One follow-up I still want separately: add v2 loop_finalize regression coverage for the same empty-content behavior, because this PR touches both code paths but only tests the v3 FinalizeStage path. Not a merge blocker given the change is direct and small.
Summary
"..."fallback ở v2 (internal/agent/loop_finalize.go) và v3 (internal/pipeline/finalize_stage.go) finalize.FinalContentnhư silent reply (suppress delivery, không leak"..."xuống channel).slog.Warnriêng cho case empty (kèm session/iteration/thinking_len/tool_calls/async_tool_calls) để root cause LLM trả rỗng không bị che bởi NO_REPLY.Why
Trace evidence (
019e01c2-b12d-7bf7-babc-29833b288f81, agenttra, model qwen3.6-plus qua bailian):traces.output_previewvàsessions.messages[].contentđều lưu literal"..."."..."2 lần liên tiếp trong group Zalo.Root cause proximate là fallback ở finalize:
if FinalContent == "" → "...". Bug ảnh hưởng tất cả channel (Telegram/Discord/Zalo/Feishu/WhatsApp/Slack), không Zalo-specific.Downstream consumer (
cmd/gateway_consumer_normal.go:468,cmd/gateway_announce_queue.go:137) đã có sẵn checkContent == "" || IsSilentReply(...)để suppress + cleanup placeholder. Fallback"..."đang bypass logic này. Fix này feed đúng empty xuống path đó.What changed
internal/pipeline/finalize_stage.goisSilentByTokenvsisEmptyOutput, gộp thànhisSilent; bỏ branch fallback"..."; log warn cho emptyinternal/agent/loop_finalize.gointernal/pipeline/stages_test.goTest plan
go build ./...cleango build -tags sqliteonly ./...clean (desktop edition)go vet ./...cleango test ./internal/pipeline/ ./internal/agent/passTestFinalizeStage_EmptyContent_SuppressesInsteadOfEllipsis,TestFinalizeStage_NoReplyToken_StillSuppresses,TestFinalizeStage_RealContent_PassesThrough)agent produced empty final content; suppressing deliveryxuất hiện và channel KHÔNG gửi"..."Unresolved
FinalContentrỗng dù LLM trả 596 output tokens) chưa giải - fix này chặn symptom rò ra user, không sửa nguyên nhân Content rỗng. Cần điều tra parser bailian/qwen3.6-plus (có thể đẩy text vàoreasoning_contentfield) qua log warn mới khi reproduce. Tracking riêng nếu confirm.Behavior change cho consumer
Sau fix:
→ channel nhận
Content=""→ không send tin nhắn"..."nữa. Placeholder cleanup vẫn chạy bình thường nếu channel có streaming/placeholder.