Skip to content

fix(agent): suppress empty final content instead of sending literal "..."#1116

Open
nguyennguyenit wants to merge 1 commit into
mainfrom
fix/agent-empty-final-content-no-ellipsis
Open

fix(agent): suppress empty final content instead of sending literal "..."#1116
nguyennguyenit wants to merge 1 commit into
mainfrom
fix/agent-empty-final-content-no-ellipsis

Conversation

@nguyennguyenit

Copy link
Copy Markdown
Contributor

Summary

  • Bỏ hardcoded "..." fallback ở v2 (internal/agent/loop_finalize.go) và v3 (internal/pipeline/finalize_stage.go) finalize.
  • Coi empty FinalContent như silent reply (suppress delivery, không leak "..." xuống channel).
  • Thêm slog.Warn riê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.
  • NO_REPLY token semantics giữ nguyên.

Why

Trace evidence (019e01c2-b12d-7bf7-babc-29833b288f81, agent tra, model qwen3.6-plus qua bailian):

  • LLM trả 596 output tokens với câu trả lời tiếng Việt đầy đủ (~700 chars).
  • Nhưng traces.output_previewsessions.messages[].content đều lưu literal "...".
  • User thấy bot reply "..." 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 check Content == "" || IsSilentReply(...) để suppress + cleanup placeholder. Fallback "..." đang bypass logic này. Fix này feed đúng empty xuống path đó.

What changed

File Δ What
internal/pipeline/finalize_stage.go +14/-6 Tách isSilentByToken vs isEmptyOutput, gộp thành isSilent; bỏ branch fallback "..."; log warn cho empty
internal/agent/loop_finalize.go +18/-15 Same logic cho v2 path (predefined agents)
internal/pipeline/stages_test.go +70 3 regression tests: empty→suppress, NO_REPLY→suppress, real→passthrough

Test plan

  • go build ./... clean
  • go build -tags sqliteonly ./... clean (desktop edition)
  • go vet ./... clean
  • go test ./internal/pipeline/ ./internal/agent/ pass
  • 3 regression tests pass (TestFinalizeStage_EmptyContent_SuppressesInsteadOfEllipsis, TestFinalizeStage_NoReplyToken_StillSuppresses, TestFinalizeStage_RealContent_PassesThrough)
  • Reproduce trên staging: trigger run với input gần context_window, kiểm tra log agent produced empty final content; suppressing delivery xuất hiện và channel KHÔNG gửi "..."

Unresolved

  • Root cause gốc (vì sao FinalContent rỗ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ào reasoning_content field) qua log warn mới khi reproduce. Tracking riêng nếu confirm.

Behavior change cho consumer

Sau fix:

WARN agent produced empty final content; suppressing delivery
  session=agent:tra:tra-on-zalo:group:7937974080316500889
  iteration=1 thinking_len=1300 tool_calls=0 async_tool_calls=0

→ 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.

…..."

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 mrgoonie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mrgoonie added agent:github-maintain Processed by github-maintain automation maintain:triaged Triaged by maintain workflow status:blocked Blocked by external dependency or decision labels Jun 25, 2026

@mrgoonie mrgoonie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mrgoonie

Copy link
Copy Markdown
Contributor

Cron-safe recheck: latest head 18a9eb7c is still approved in intent, but GitHub still reports mergeStateStatus=DIRTY. CI on the old head is green, but this cannot be merged until the branch is rebased/updated against the current base and conflicts are resolved. No merge attempted in this run.

@mrgoonie mrgoonie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mrgoonie

Copy link
Copy Markdown
Contributor

Cron-safe recheck: latest head 18a9eb7c remains approved in intent, but GitHub still reports mergeStateStatus=DIRTY. CI shown on the old head is green, but this PR cannot be merged until the branch is updated/rebased and conflicts are resolved. No merge attempted in this run.

@mrgoonie

Copy link
Copy Markdown
Contributor

Metadata refresh from github-maintain: I rechecked this PR and it is still valuable, but it remains non-mergeable because GitHub reports mergeStateStatus=DIRTY against main.

Review status:

  • Duplicate/prior implementation: no duplicate PR found for empty FinalContent / literal ellipsis suppression. Related prior work exists in fix: unify NO_REPLY detection #1233 / commit 6c8ad128 for NO_REPLY detection, but that does not remove the empty-content "..." fallback.
  • Project standards: scope is tight and matches existing consumer behavior; cmd/gateway_consumer_normal.go already suppresses empty/NO_REPLY replies and can clean placeholders when content is empty.
  • Strategic necessity: clear value — prevents leaking literal "..." to user-facing channels and adds regression coverage.

Current blocker:

  • internal/agent/loop_finalize.go and internal/pipeline/finalize_stage.go still have fallback branches that turn empty content into "..." on the base branch. This PR fixes that, but it must be rebased/updated because merge state is dirty.
  • Existing checks on the old head SHA were green, but they are stale after base drift. Please rebase/update onto current main and rerun CI.

No approval/merge attempted in this run because the PR is dirty and not merge-ready.

@mrgoonie mrgoonie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:github-maintain Processed by github-maintain automation maintain:triaged Triaged by maintain workflow status:blocked Blocked by external dependency or decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants