Skip to content

fix(models): surface terminal ChatStream errors to callers and scan status - #702

Merged
boy-hack merged 1 commit into
Tencent:mainfrom
helo060228:fix/chatstream-terminal-errors
Oct 14, 2026
Merged

boy-hack merged 1 commit into
Tencent:mainfrom
helo060228:fix/chatstream-terminal-errors

Conversation

@helo060228

Copy link
Copy Markdown
Contributor

Summary

Fixes #701. Implements option 1 from the discussion, on top of the now-merged #695.

ChatStream returns one channel of ChatStreamEvent{Text, Err}. Text deltas arrive as before. A failed stream emits one terminal error event with empty text, then closes; a successful stream closes without an error event. Retry exhaustion and context cancellation also produce a terminal error. Retries before text and the abort-after-text behavior from #695 are preserved.

The four consumers now check errors:

  • ChatResponse returns the text accumulated so far and the original error.
  • runner.Analysis preserves the screenshot and partial response, returns the error, and does not parse the aborted response into a vulnerability.
  • SummaryChat returns partial text and a wrapped error.
  • SummaryResult returns an error before parsing any issues, while keeping the emitted text in logs.

The task layer reports interrupted analysis through the existing websocket-visible failed status and preserves partial text in the failure detail. It no longer sends a deferred completed status over that failure. The related model-generated scan summaries also check errors instead of storing truncated output as a successful summary.

This keeps valid base scan findings available. A concurrency-safe set counts failed targets once even when several analysis stages fail for the same target. Both final completion messages and the completed-tool description explicitly report incomplete analysis/results when that count is nonzero, while the affected analysis substep remains failed. The scan can return its partial results; one model failure does not discard all scan findings. No REST or websocket schema changes are needed.

Type of change

  • Bug fix

How was this tested?

Using Go 1.25.1 on Linux:

  • go build ./...: passed.
  • go vet ./...: passed.
  • go test ./... -count=1 -timeout=120s: 16 packages passed; the existing common/fingerprints/parser.TestSingleRuleForParse failed. This test makes a live request to https://security.tencent.com/index.php and dereferences the response after an unsuccessful request. The same failure was reproduced on unmodified main at 3eb7b466.
  • go test ./... -skip '^TestSingleRuleForParse$' -count=1 -timeout=120s: all 17 test-bearing packages passed.
  • Focused model, runner, and task tests passed with -race.
  • Focused MCP tests passed normally and across 30 repetitions. With -race, they expose an existing shared-state race in internal/gologger.getCaller (minimumCallerDepth). A separate two-logger reproduction on unmodified main reproduced that race; no unrelated logger fix is included here.
  • git diff --check: passed.

Regression coverage includes clean and empty completion, interrupted streams with partial text, no replay after emitted text, retry exhaustion, cancellation before/after output and during backoff, token accounting before terminal delivery, failed reports whose tags happen to look complete, exactly one terminal task status, and zero/one/concurrent target failures with deduplicated English/Chinese partial-completion warnings.

Related issue

Fixes #701

Compatibility and limits

  • The Go channel element type changes from string to ChatStreamEvent; repository call sites are migrated together.
  • Callers must keep consuming until closure or the terminal error, including after cancellation. Stopping consumption without draining can leave the producer blocked.
  • Transport error detection follows the SDK. This does not add validation for a server that ends an otherwise well-formed HTTP response without an SSE completion marker.
  • Existing usage-only chunk accounting and shared logger concurrency behavior are outside this change.
  • No live provider or browser end-to-end test was run; failure status propagation is covered at the callback boundary.

Checklist

  • go build ./... succeeds locally
  • I followed the existing file/rule format conventions for this contribution type
  • I have not introduced hardcoded secrets or credentials

@boy-hack

Copy link
Copy Markdown
Collaborator

Verified locally on head bce928f8 (isolated worktree):

  • go build ./... / go vet ./... clean
  • Affected packages all pass: common/utils/models (incl. the extended stream-retry suite), common/runner, internal/mcp(+utils), common/agent, common/websocket
  • Full go test ./...: zero failures, matching the green main baseline

Review notes:

  • Terminal-error semantics match the option-1 design agreed in ChatStream callers cannot distinguish a clean completion from a mid-stream abort #701: exactly one ChatStreamEvent{Err} on failure/cancellation/retry-exhaustion, then channel closure; clean completion closes without an error event. Well documented on the struct.
  • fix(models): abort instead of retrying after ChatStream emitted partial output #695's abort-after-partial-output behavior is preserved, and the select on send closes the producer cleanly if a consumer stops draining (no goroutine leak).
  • Consumers: ChatResponse returns accumulated text + the original error; runner.Analysis keeps the screenshot and partial response but no longer parses an aborted report into a vulnerability; both MCP summary paths wrap and propagate; tasks.go records per-target analysis failures and surfaces AgentStatusFailed plus the new bilingual partial-completion status instead of silently dropping errors.
  • Bonus improvements: token accounting centralized in the producer defer, stream.Close() on every path, and tasks.go summaries now use the request ctx instead of context.Background().

No change requests. All four ChatStream call sites are migrated in this PR and no other implementors exist. CI green on head. LGTM — this closes #701 as designed.

@boy-hack
boy-hack marked this pull request as ready for review October 14, 2026 01:32
@boy-hack
boy-hack merged commit 1fa464c into Tencent:main Oct 14, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ChatStream callers cannot distinguish a clean completion from a mid-stream abort

2 participants