Skip to content

feat(sight): add conversation grader API#1395

Merged
jfeng18 merged 3 commits into
alibaba:mainfrom
haoowa:feat/sight/grader-api
Jul 9, 2026
Merged

feat(sight): add conversation grader API#1395
jfeng18 merged 3 commits into
alibaba:mainfrom
haoowa:feat/sight/grader-api

Conversation

@haoowa

@haoowa haoowa commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Description

Add the AgentSight manual conversation grader backend. The API evaluates persisted GenAI and interruption evidence with a deterministic rule grader, stores idempotent evaluation_runs in SQLite, and exposes evaluate/latest endpoints for dashboard clients.

The store is initialized once in AppState, interruption evidence is loaded from the existing InterruptionStore, and JSON conversion work stays outside the SQLite mutex. This PR also carries the narrow AgentSight docs for the grader API and includes current-toolchain mechanical clippy fixes needed for independent -D warnings validation.

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 grader --locked --quiet
  • cargo clippy -q -p agentsight --all-targets --locked -- -D warnings

Additional Notes

Split from #1349 following review feedback. The dashboard controls are intentionally left to a separate PR.

@github-actions github-actions Bot added component:sight src/agentsight/ scope:documentation ./docs/|./*.md|./NOTICE labels 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

Nice to see the grader land as an independently reviewable unit — the scoring/verdict math itself checks out. A few observations:

Test coverage of the core contracts

  • weighted_score (rule.rs:32-40) has no discriminating test: the only score assertion is >= 0.8 on an all-1.0 dimension vector, which round_score clamps — so weight changes and the 0.5/0.8 Pass/Warn/Fail cutoffs aren't exercised (e.g. an efficiency 0.10→0.50 mutation survives the suite).
  • grader/storage.rs (the INSERT OR IGNORE idempotency contract), POST /grader/evaluate (reused_existing_run / re-query branches, 409/404/400 mappings), and compute_input_hash (the idempotency key) have no direct tests.

Two internal-consistency points

  • select_root_cause (rule.rs:312-360) maps only a subset of codes; rate_limit / auth_error / context_overflow / token_limit / retry_storm / dead_loop fall through to RootCause::None, whose as_str() is "none" — surfacing summaries like "usable but needs review for none." (verdict/score stay correct.)
  • score_safety (rule.rs:217) filters on interruption_type.contains("safety") without the !resolved check that score_runtime_health and build_findings use, so an already-resolved safety interruption still sets safety=0.0/Fail and subtracts 0.10 while emitting no safety finding or SafetyRisk root cause.

Minor: the four mechanical clippy rewrites (probes.rs / sslsniff.rs / storage/sqlite/token.rs / storage/unified.rs) are bundled here and also ride in #1393/#1394 — not a merge hazard (identical blobs), just extra review surface.

@haoowa

haoowa commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Updated this PR in 6c246162.

What changed in this follow-up:

  • Added direct regression coverage for the grader scoring contract:
    • weighted score math
    • Pass / Warn / Fail score cutoffs
    • compute_input_hash stability and input-change behavior
    • EvaluationStore::insert_completed idempotency via INSERT OR IGNORE
    • POST /grader/evaluate reuse/error branches, including existing-run reuse and 400 / 404 / 409 mappings
  • Fixed select_root_cause so the reviewed interruption codes no longer fall through to RootCause::None:
    • rate_limit, auth_error, context_overflow, token_limit -> RuntimeError
    • retry_storm, dead_loop -> LoopDetected
  • Fixed score_safety to ignore already-resolved safety interruptions, matching the unresolved-only behavior used by runtime health and findings.
  • Modeled grader in the AgentSight architecture boundary checker as an L7 Serve module:
    • allowed server -> grader
    • allowed grader -> storage
    • updated docs/ARCHITECTURE.md to keep the documented layer graph in sync

Validation run on Linux:

  • cargo fmt --all -- --check
  • cargo clippy --all-targets -- -D warnings
  • cargo test --quiet -- --test-threads=1
  • python3 scripts/check-arch-boundaries.py

The four mechanical clippy rewrite files mentioned as extra review surface were not changed in this follow-up.

@haoowa haoowa force-pushed the feat/sight/grader-api branch from 6c24616 to 8275385 Compare July 9, 2026 09:27
haoowa added 2 commits July 9, 2026 17:40
Add a manual rule-based conversation grader over GenAI and interruption
evidence with idempotent SQLite persistence.

Keep the MVP backend-only. API handlers expose evaluate/latest endpoints,
while evaluation runs reuse the shared server store and injected interruption
store. Rebased with dashboard auth by wiring test AppState helpers with both
the evaluation store and disabled auth state.

Assisted-by: Codex:GPT-5
Signed-off-by: wanghao <1562495626@qq.com>
Resolved safety interruptions no longer affect the safety score. Interruption
root-cause mapping now covers provider, context, token, retry, and loop signals
called out in review.

Add regression coverage for score weighting, verdict cutoffs, input hash
stability, storage idempotency, evaluate API reuse/error branches, and the
grader architecture boundary.

Assisted-by: Codex:GPT-5
Signed-off-by: wanghao <1562495626@qq.com>
@haoowa haoowa force-pushed the feat/sight/grader-api branch from 8275385 to f36d6f2 Compare July 9, 2026 09:42
@haoowa

haoowa commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Added a follow-up update for the required check failure.

This latest push was specifically to address the Test agentsight gate failure
after main advanced. The failing step was the CI lint command:

  • cargo clippy --all-targets -- -D warnings

Root cause:

  • main added pending_origin and pending_match_key to PendingCallInfo.
  • This PR's grader API test helper still initialized PendingCallInfo with the
    older field set, so the test target failed to compile in CI.

Fix pushed in the latest head f36d6f24:

  • Rebased this PR onto the current main.
  • Updated the pending conversation test fixture to set
    pending_origin: PendingOrigin::RequestCapture.
  • Set pending_match_key: None for that request-capture fixture.
  • Folded the change into the grader API commit so each PR commit remains
    independently buildable.

Validation:

  • Linux test host: cargo fmt --all -- --check
  • Linux test host: python3 scripts/check-arch-boundaries.py
  • Linux test host: cargo clippy --all-targets -- -D warnings
  • Linux test host: cargo test --quiet -- --test-threads=1
  • GitHub Actions: Test agentsight is now passing

@jfeng18

jfeng18 commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Round-2 fresh pass on the updated PR (all 5 round-1 findings confirmed fixed — thanks for the thorough response). This pass looked at the whole grader, not just the delta. Two new findings worth addressing before merge:

1. tool_response_has_error ignores is_error: false (evidence.rs:66-93)

tool_response_has_error early-returns true on is_error == Some(true), but when is_error == Some(false) it does not early-return false — it falls through to value_has_error_signal, which substring-scans response/content/error keys for "failed", "error:", "exception", "exit code 1", "traceback", "permission denied", "command not found", etc.

Common successful coding-agent tool output hits this: a pytest summary "5 passed, 2 failed" (matches "failed"), a grep through logs (matches "error:"), a build summary with warnings. When flagged, score_tool_use drops to 0.45, a tool_failure finding is emitted, and root_cause can become ToolFailure — downgrading a clean conversation to Warn. This is a high-frequency false-positive class for the grader's primary use case (coding-agent conversations). Reproduced with a standalone harness: {content: "5 passed, 2 failed", is_error: false} → flagged = true.

2. select_verdict can return Pass while root_cause is non-None and a dimension is Fail (rule.rs:380-402)

select_verdict only hard-fails on NoFinalAnswer / InterruptedMainCall; otherwise it returns Pass when score >= 0.8 and no finding has severity != "low". It never consults root_cause or per-dimension verdicts.

Concrete input: a single completed event with usable output and total_tokens = 250,000 (no interruptions, no tool failures). Trace:

  • efficiency dimension = 0.35 (Fail) — 250k ≥ 200k threshold
  • all other dimensions = 1.0
  • weighted_score = 0.35×1.0 + 0.25×1.0 + 0.20×1.0 + 0.10×0.35 + 0.10×1.0 = 0.94 (Pass threshold is 0.8)
  • root_cause = ExcessiveCost (efficiency < 0.5, rule.rs:370)
  • findings = [] (build_findings only emits from unresolved interruptions, not from dimension scores)
  • select_verdict: not NoFinalAnswer/InterruptedMainCall → score 0.94 ≥ 0.8 → findings empty → Pass

Result: verdict=Pass, root_cause=ExcessiveCost, efficiency=Fail(0.35), summary="Conversation completed successfully with no deterministic quality issue.", action="No immediate action required." — self-contradictory.

Four non-blocking notes (not gating merge):

  • select_root_cause now checks RuntimeError before SafetyRisk, so a dual-unresolved {rate_limit, safety_filter} classifies as RuntimeError instead of SafetyRisk — safety finding stays in findings[] but headline label is demoted. (low, requires specific dual-interruption input)
  • context_overflow/token_limit → RuntimeError's recommended action advises "retry" which doesn't fit a deterministic limit condition. (nit, advisory text)
  • "Pending call" evidence refs use first_event_refs (oldest event) rather than the actual pending event — deeplink highlights the wrong call. (low, grade value correct)
  • evaluate endpoint loads full conversation + hashes before checking the idempotency cache. (low, optional optimization)

Respect explicit tool result error flags before textual heuristics.

Downgrade non-hard root causes and failed dimensions to warn verdicts.

This avoids clean passes when deterministic evidence still requires review.

High-cost conversations with usable output remain warnings instead of hard failures.

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

haoowa commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Updated in d9564c2 to address the two blocking grader notes:

  • Tool error detection now treats explicit structured is_error values as authoritative. is_error: false no longer falls through to textual heuristics, while is_error: true and missing flags preserve the previous detection behavior.
  • Top-level verdict selection now considers non-None root causes and failed dimensions. Non-hard issues such as excessive cost now produce Warn instead of a clean Pass, while hard no-output/interrupted cases still fail.

Added regression coverage for both cases:

  • is_error: false with content like 5 passed, 2 failed
  • high-token completed conversations with ExcessiveCost / failed efficiency dimension

Validated on Linux test host:

  • cargo fmt --all -- --check
  • python3 scripts/check-arch-boundaries.py
  • cargo clippy --all-targets -- -D warnings
  • cargo test --quiet -- --test-threads=1 (1053 passed)

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

Both blocking items from round 2 are addressed in d9564c21 — confirmed by reading the fix and CI (1053 tests pass):

  1. is_error: false now authoritative (evidence.rs:67-69): tool_response_has_error returns is_error directly when present (both true and false), only falling through to text heuristics when the field is absent. The recursive value_has_error_signal has the same guard on nested objects. A pytest summary like "5 passed, 2 failed" with is_error: false no longer triggers a tool_failure — verified the test at evidence.rs:284.

  2. select_verdict no longer contradicts root_cause / failed dimensions (rule.rs:380-407): now checks root_cause != RootCause::None and has_failed_dimension in the Warn gate. The 250k-token ExcessiveCost scenario (efficiency=0.35/Fail, weighted=0.94) correctly produces Warn instead of Pass.

LGTM on the grader.

@jfeng18 jfeng18 merged commit a57c3d9 into alibaba:main Jul 9, 2026
17 checks passed
@haoowa haoowa deleted the feat/sight/grader-api branch July 9, 2026 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component:sight src/agentsight/ scope:documentation ./docs/|./*.md|./NOTICE

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants