Skip to content

fix(sight): avoid false interruption signals#1393

Merged
jfeng18 merged 2 commits into
alibaba:mainfrom
haoowa:fix/sight/interruption-signals
Jul 9, 2026
Merged

fix(sight): avoid false interruption signals#1393
jfeng18 merged 2 commits into
alibaba:mainfrom
haoowa:fix/sight/interruption-signals

Conversation

@haoowa

@haoowa haoowa commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Description

Avoid false interruption records when a successful assistant response mentions troubleshooting words such as timeout, auth, or rate limit. The detector now only scans structured provider error text and non-2xx response bodies for runtime error classifications.

This PR is split out of #1349 so the interruption classifier fix can be reviewed independently. It also includes the current-toolchain mechanical AgentSight clippy fixes required for this split PR to pass -D warnings independently.

Related Issue

Refs #1304

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional change)
  • Performance improvement
  • CI/CD or build changes

Scope

  • cosh (copilot-shell)
  • cosh-ng (cosh-ng)
  • sec-core (agent-sec-core)
  • skill (os-skills)
  • sight (agentsight)
  • tokenless (tokenless)
  • ckpt (ws-ckpt)
  • memory (agent-memory)
  • anolisa (anolisa-cli)
  • skillfs (SkillFS)
  • Multiple / Project-wide

Checklist

  • I have read the Contributing Guide
  • My code follows the project's code style
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the documentation accordingly
  • For cosh: Lint passes, type check passes, and tests pass
  • For cosh-ng: cargo clippy --all-targets -- -D warnings and cargo fmt --check pass
  • For sec-core (Rust): cargo clippy -- -D warnings and cargo fmt --check pass
  • For sec-core (Python): Ruff format and pytest pass
  • For skill: Skill directory structure is valid and shell scripts pass syntax check
  • For sight: cargo clippy -- -D warnings and cargo fmt --check pass
  • For tokenless: cargo clippy -- -D warnings and cargo fmt --check pass
  • For memory (Linux only): cargo clippy --all-targets -- -D warnings, cargo fmt --check, and cargo test pass
  • For anolisa: cargo clippy --all-targets --locked -- -D warnings, cargo fmt --all --check, and cargo test --locked pass
  • For skillfs: cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace pass
  • Lock files are up to date (package-lock.json / Cargo.lock)

Testing

Ran on the Linux AgentSight test host from an archive of this split branch:

  • cargo fmt --all -- --check
  • cargo test -p agentsight interruption::detector --locked --quiet
  • cargo clippy -q -p agentsight --all-targets --locked -- -D warnings

Additional Notes

Split from #1349 following review feedback to keep capture fixes and the grader feature reviewable in smaller units.

@haoowa haoowa requested review from chengshuyi and jfeng18 as code owners July 8, 2026 07:58
@github-actions github-actions Bot added the component:sight src/agentsight/ label Jul 8, 2026
@haoowa haoowa mentioned this pull request Jul 8, 2026
33 tasks
@jfeng18

jfeng18 commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Thanks for splitting this out — the false-positive suppression goal makes sense. One concern on the recall side:

After this change combined_error only folds in the response body when status_code >= 400 (detector.rs:107-112), and call.error is itself only populated for status_code >= 400 (call_builder.rs:137). So for an HTTP 200 stream that commits the status line and then emits a mid-stream error envelope, both inputs to combined_error are empty — none of the AuthError/RateLimit/ServiceUnavailable/ContextOverflow keyword rules, nor the generic LlmError fallback (detector.rs:284), can fire.

Concretely: Anthropic Messages streaming returns 200 then event: error / {"type":"error","error":{"type":"overloaded_error"}}; OpenAI-compatible proxies stream {"error":{...}} chunks at 200. These land in raw_body and were classified before this PR (e.g. ServiceUnavailable). After it they are either reclassified as SseTruncated (when duration ≥ sse_min_duration_ns) or, for fast-fail (<1s) errors, produce no interruption event at all. Since agents almost always stream, this looks like the common interruption path.

(Reproduced pre/post against the SSE parse path.) Flagging in case the 200-status-with-error-body case is collateral rather than intended.

@haoowa haoowa force-pushed the fix/sight/interruption-signals branch from 966b627 to a04de87 Compare July 9, 2026 03:30
haoowa added 2 commits July 9, 2026 11:37
Only provider error text and non-2xx response bodies feed runtime interruption classification.

Successful assistant output may mention timeout, auth, or rate-limit troubleshooting.

Those phrases should not create captured interruption events by themselves.

Assisted-by: Codex
Signed-off-by: wanghao <1562495626@qq.com>
Parse structured error envelopes from successful SSE streams before truncated-stream detection.

Keep ordinary HTTP 200 assistant text out of keyword matching.

Restore recall for provider errors emitted after the stream status line is committed.

The detector only trusts explicit error-shaped events.

Assisted-by: Codex
Signed-off-by: wanghao <1562495626@qq.com>
@haoowa haoowa force-pushed the fix/sight/interruption-signals branch from a04de87 to 9d1e8e2 Compare July 9, 2026 03:40
@jfeng18

jfeng18 commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Thanks — this closes the recall regression I raised in round 1. I verified by building and running this time, not just reading the diff:

  • cargo test --lib interruption::detector42 passed / 0 failed; both new tests (test_detects_anthropic_sse_error_event_in_200_stream, test_detects_structured_sse_error_object_in_200_stream) green.
  • Mutation-checked the fix: neutralizing detector.rs stream_error extraction to None flips both tests (detected 0 vs expected 1), so they genuinely gate the fix rather than passing formally.

Also confirmed my main worry does not happen: a >1s 200-SSE error stream is not double-classified — rule 7 (stream_error.is_some()) emits one event and returns before the SseTruncated rule is reached, so exactly one event fires.

Two non-blocking notes for later (not for this PR):

  1. Extraction is gated on status_code < 400 && is_sse, so a non-SSE HTTP 200 carrying a buffered (non-streamed) error envelope is still not scanned — same recall class, different transport. Probably a separate follow-up rather than widening this diff.
  2. The test_ignores_* false-positive guards all run with is_sse=false, so they don't exercise the new SSE stream_error path; the behavior is correct today but not locked by a committed test.

LGTM on the recall fix.

@jfeng18 jfeng18 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Recall regression from round 1 is closed and verified by build + tests + mutation (42 passed; neutralizing the fix flips the two new SSE tests). No blockers; the two notes above are non-blocking follow-ups. LGTM.

@jfeng18 jfeng18 merged commit dfa082c into alibaba:main Jul 9, 2026
17 checks passed
@haoowa haoowa deleted the fix/sight/interruption-signals branch July 9, 2026 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component:sight src/agentsight/

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants