Skip to content

fix(sight): persist idle streams and tool results#1394

Merged
jfeng18 merged 2 commits into
alibaba:mainfrom
haoowa:fix/sight/stream-capture-tool-results
Jul 9, 2026
Merged

fix(sight): persist idle streams and tool results#1394
jfeng18 merged 2 commits into
alibaba:mainfrom
haoowa:fix/sight/stream-capture-tool-results

Conversation

@haoowa

@haoowa haoowa commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Description

Persist idle in-flight streamed calls so manually interrupted conversations remain visible as pending evidence instead of being silently dropped. This also preserves Anthropic tool_result blocks as structured input message parts so downstream scoring can distinguish actual tool failures from historical raw context.

This PR also adds regression coverage for oversized request-body eviction and 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 aggregator --locked --quiet
  • cargo test -p agentsight genai::call_builder --locked --quiet
  • cargo clippy -q -p agentsight --all-targets --locked -- -D warnings

Additional Notes

Split from #1349 following review feedback to keep capture fixes separate from the grader API and dashboard work.

Drain idle in-flight streamed calls so interrupted conversations remain
visible as pending evidence.

Preserve Anthropic tool_result blocks as structured input messages and add
coverage for oversized request-body eviction.

Assisted-by: Codex
Signed-off-by: wanghao <1562495626@qq.com>
@haoowa haoowa requested review from chengshuyi and jfeng18 as code owners July 8, 2026 07:59
@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

Re-checked this one closely since it's timing-bounded — it holds, with a scoping note.

New in this PR: drain_and_persist_idle_connections (unified.rs:1287) persists a fresh-generate_id() pending row for a live process once a stream idles past idle_timeout (60s, config.rs:61), and pops the connection from the aggregator. Since complete_pending keys on the real call_id, that orphan row is never resolved even if the stream resumes and completes successfully.

Consequence: ProcMonEvent::Exit carries no exit code (procmon.rs:47-53), so handle_agent_crash_detection (unified.rs:861) fires on every tracked-agent exit and sweeps status='pending' rows for the pid into InterruptionType::AgentCrash (Critical, types.rs:77), timestamped at exit. So a stream that idled >60s, resumed and completed fine, followed by a clean /exit within ~[t0+60s, t0+300s], yields a spurious Critical agent_crash on a successful session. After ~t0+300s the stale scanner flips the row to interrupted and it no longer trips the sweep, so the window is ~4 min.

Attribution, to be fair: the exit-without-exit-code gate and "pending row at clean exit → agent_crash" behavior already exist on main; this PR doesn't introduce those. What's new is a durable, unresolvable pending row for a successful idled-then-resumed stream, which previously left no row at all. Flagging since a false Critical on a clean exit is costly for a detection product; the deeper "Exit has no exit code" limitation looks pre-existing and probably a separate discussion.

Idle SSE persistence now writes source metadata and a deterministic pending
match key so timeout snapshots stay distinguishable from crash drains.

The aggregator snapshots idle connections without removing in-memory stream
state. Resumed streams can promote the stored pending row instead of losing
the active SSE flow.

The match key uses request metadata and body bytes. If the parser cannot
reconstruct the same request shape, completion still falls back to the
existing insert path.

Assisted-by: Codex:GPT-5
Signed-off-by: wanghao <1562495626@qq.com>
@jfeng18

jfeng18 commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

The idle-drain fix closes the finding I raised — a clean process exit no longer sweeps idle-drained rows into a spurious Critical agent_crash. I built and ran it rather than just reading:

  • cargo test --lib storage::sqlite::genai51 passed / 0 failed; test_crash_sweep_ignores_idle_drain_pending, test_list_pending_for_pid(s), and test_complete_pending_promotes_idle_snapshot_by_match_key all green.
  • Mutation-checked the gate: removing the AND pending_origin != 'idle_drain' filter from list_pending_for_pid flips test_crash_sweep_ignores_idle_drain_pending (2 rows vs expected 1), so the exclusion is genuinely what prevents the false crash.

Both halves of the original concern are addressed: the PendingOrigin::IdleDrain tag + its exclusion from the crash sweep stops the spurious agent_crash, and pending_match_key + the non-destructive snapshot let a resumed stream reconcile the row instead of it being an unresolvable orphan. Nice.

One thing blocking merge, though — CI is red on Commit Message Lint (not the tests; Test agentsight passes). From the job log:

✖  body's lines must not be longer than 100 characters [body-max-line-length]
✖  found 1 problems, 0 warnings

Both commits trip it — the bodies have lines of 146/359 chars (preserve idle stream snapshots) and 120 chars (persist idle streams and tool results). Rewrapping the two commit bodies at ≤100 chars and force-pushing should turn it green; happy to approve once CI is clean.

@haoowa haoowa force-pushed the fix/sight/stream-capture-tool-results branch from 749795a to 183a697 Compare July 9, 2026 08:57
@haoowa

haoowa commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Additional note on the code changes made for the earlier #1394 review feedback.

The review concern was that persisting idle streams could make an active stream
look like a crash-drained pending call, and that a resumed stream might not be
able to reconnect to the idle snapshot later.

The PR now addresses that by:

  • Marking idle-timeout persisted rows explicitly as idle_drain via
    pending_origin, separate from normal request capture and dead-PID drains.
  • Adding a deterministic pending_match_key so a later completed stream can
    promote the previously persisted idle snapshot instead of leaving duplicate
    or stale pending evidence.
  • Changing idle timeout handling from destructive drain to non-destructive
    snapshot, so SseActive state remains in memory and the stream can continue
    if more SSE chunks arrive.
  • Excluding idle_drain rows from crash/dead-PID pending sweeps, so a live but
    idle stream is not reclassified as an agent-crash artifact.
  • Adding SQLite schema migrations and regression tests for:
    • persisted idle origin and match key
    • promotion by match key
    • crash sweep ignoring idle snapshots
    • resumed SSE streams after idle snapshot

Validation was run on the Linux test host:

  • cargo fmt --all -- --check
  • cargo test --quiet
  • cargo clippy --all-targets -- -D warnings

The latest force-push only wrapped commit message bodies for commitlint; it did
not change the code tree from the reviewed 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.

Idle-drain finding closed and verified (build + 51 passed + mutation on the filter). CI now green after the commit-body rewrap; code is byte-identical to the version I verified. LGTM.

@jfeng18 jfeng18 merged commit 4c86326 into alibaba:main Jul 9, 2026
16 checks passed
@haoowa haoowa deleted the fix/sight/stream-capture-tool-results branch July 9, 2026 09:11
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