Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Chunked selection and response-validation paths contain correctness gaps, including invalid __none__ handling and malformed score responses.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 5
Open (8)
What changed in this PR
Adds a TypeSafe System One Jev client, typed decision models, selection middlewares, tests, documentation, and distribution integration.
Changes:
- Added HTTP client with retries, timeouts, authentication, and response validation.
- Added skill suggestion and tool selection middlewares with chunking and fail-open behavior.
- Registered the extension in Maven aggregators, BOM, all-in-one distribution, and bilingual docs.
| File | Description |
|---|---|
docs/v2/zh/integration/ecosystem/jev.md |
Chinese Jev integration guide |
docs/v2/en/integration/ecosystem/jev.md |
English Jev integration guide |
docs/docs.json |
Documentation navigation and redirects |
agentscope-extensions/pom.xml |
Registers the extension module |
agentscope-extensions/agentscope-extensions-jev/pom.xml |
Jev module dependencies |
agentscope-distribution/agentscope-bom/pom.xml |
BOM registration |
agentscope-distribution/agentscope-all/pom.xml |
All-in-one distribution registration |
.../JevClient.java |
HTTP client and validation |
.../JevException.java |
Jev exception type |
.../JevRetryPolicy.java |
Retry configuration |
.../SystemOneRequest.java |
Request model and builder |
.../SystemOneResult.java |
Response model |
.../Usage.java |
Token usage model |
.../Question.java |
Question polymorphism |
.../Answer.java |
Answer polymorphism |
.../NoulQuestion.java |
Noul question model |
.../NoulAnswer.java |
Noul answer model |
.../ChoiceQuestion.java |
Choice question model |
.../ChoiceAnswer.java |
Choice answer model |
.../ScoreQuestion.java |
Score question model |
.../ScoreAnswer.java |
Score answer model |
.../JevSelectionSupport.java |
Shared selection helpers |
.../JevSkillSuggestionMiddleware.java |
Skill suggestion middleware |
.../JevToolSelectionMiddleware.java |
Tool selection middleware |
.../JevClientTest.java |
Client behavior tests |
.../JevDtoTest.java |
DTO serialization tests |
.../JevExceptionTest.java |
Exception tests |
.../JevRetryPolicyTest.java |
Retry policy tests |
.../JevSkillSuggestionMiddlewareTest.java |
Skill middleware tests |
.../JevToolSelectionMiddlewareTest.java |
Tool middleware tests |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| private static void validateResult(SystemOneRequest request, SystemOneResult result) { | ||
| if (result.model() == null || result.model().isBlank()) { | ||
| throw new JevException("System One response model must not be blank"); | ||
| } |
| if (answer.legend() == null | ||
| || answer.probabilities() == null | ||
| || !answer.legend().keySet().equals(answer.probabilities().keySet())) { | ||
| throw new JevException( | ||
| "System One answer for question '" |
| private static String defaultApiKey() { | ||
| String apiKey = System.getenv("TYPESAFE_API_KEY"); | ||
| return apiKey != null ? apiKey : System.getenv("JEV_API_KEY"); | ||
| } |
| for (AgentSkill skill : skills) { | ||
| criteria.put(skill.getName(), skill.getDescription()); | ||
| } | ||
| criteria.put(NONE_OPTION, "No skill is needed for this request."); |
| static String topName(ChoiceAnswer answer) { | ||
| if (answer == null || answer.probabilities() == null || answer.probabilities().isEmpty()) { | ||
| return null; | ||
| } | ||
| return answer.probabilities().entrySet().stream() | ||
| .filter(entry -> !NONE_OPTION.equals(entry.getKey())) | ||
| .filter(entry -> entry.getValue() != null) | ||
| .max(Map.Entry.comparingByValue()) | ||
| .map(Map.Entry::getKey) | ||
| .orElse(null); | ||
| } | ||
| } |
| return jevCall.apply(request) | ||
| .flatMap( | ||
| result -> { | ||
| // 从每块选出一个代表，进入 shortlist |
| JevClient client = | ||
| JevClient.builder() | ||
| .apiKey(System.getenv("TYPESAFE_API_KEY")) |
| JevClient client = | ||
| JevClient.builder() | ||
| .apiKey(System.getenv("TYPESAFE_API_KEY")) |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Adds a new agentscope-extensions-jev module: a typed client for TypeSafe's System One endpoint (JevClient + sealed Question/Answer DTOs + JevRetryPolicy), plus two middlewares that use it to rank skills (JevSkillSuggestionMiddleware) and shrink the tool schema sent to the primary model (JevToolSelectionMiddleware), with docs in both languages. ~3.3k lines, all additive, with 1.2k lines of tests — good coverage of validation, retry and fail-open paths, and the module is registered in the extensions aggregator, agentscope-all and the BOM.
Overall this looks well-built and I'd land it after a round of fixes on the runtime-behaviour items below. My main theme: the remote call is on the reasoning hot path — one (or two, for >254 candidates) round trips per reasoning step with a per-attempt timeout that is restarted by retryWhen, so worst-case latency added to a ReAct turn is well over a minute before fail-open engages, and a low-confidence answer degrades to zero optional tools rather than to "no filtering".
Findings
- [Critical]
middleware/JevToolSelectionMiddleware.java:160— empty selection (Set.of(), also from theconfidenceThresholdpath inJevSelectionSupport.selectedNames) removes every optional tool for that step, which is the opposite of thefailOpenintent. - [Warning]
middleware/JevToolSelectionMiddleware.java:106— latency budget: 2 sequentialsystemOne()calls per step, each3 x timeout + backoff(defaultJevRetryPolicygives ~19.5s, i.e. ~39s worst case). Suggest an outer total deadline plus per-turn memoization. - [Warning]
middleware/JevSelectionSupport.java:57— rawMsgobjects are serialized through the SNAKE_CASEJevClient.MAPPER, so message JSON on the wire is not the canonical AgentScope shape; it also ships full conversation content (incl. tool results) to a third party with no projection/redaction hook. - [Warning]
middleware/JevToolSelectionMiddleware.java:52—load_skill_through_path/reset_tools/generate_responseare literals owned bySkillToolFactoryandReActAgent.STRUCTURED_OUTPUT_TOOL_NAME; a rename upstream silently removes them from the schema. - [Warning]
middleware/JevSkillSuggestionMiddleware.java:223—repository.getAllSkills()runs per agent invocation; blocking I/O fan-out on the hot path, worth caching. - [Warning]
JevClient.java:310— nullusageaborts an otherwise-valid response; it is a reporting field, treat it as optional. - [Warning]
JevClient.java:403— absolute1e-6probability-sum tolerance over up to 255 options will reject legitimately rounded responses. Scale the tolerance with option count or normalize before comparing. - [Info]
JevClient.java:95—systemOneBlockingusesblock(), which throws onNonBlockingthreads; guard or document. - [Info]
middleware/JevSkillSuggestionMiddleware.java:231— swallowedRuntimeExceptionwith no logger makes a permanently broken repository indistinguishable from "no skills". - [Info]
docs/docs.json:331— the page is registered but missing from the Ecosystem list indocs/v2/{en,zh}/integration/overview.md.
Suggestions
For the hot-path items, a shape that would address both at once:
return selectTools(state, optionalTools)
.timeout(totalBudget) // hard ceiling across retries
.onErrorResume(e -> failOpen ? Mono.just(allNames) : Mono.error(e))
.map(names -> names.isEmpty() ? allNames : names) // "unsure" => no filteringand in retrySpec, Retry.backoff(...).handle(...)-style accounting or an outer .timeout() so timeout bounds the whole call rather than each attempt.
Notes
- I did not build or run the module locally (no local checkout sync in this run), so the review is static: the findings above come from reading the diff against
agentscope-core'sHttpTransport/MiddlewareBase/ skill contracts. - Nice touches: sealed DTOs with explicit Jackson subtype names, strict request/response validation,
failOpenas a builder flag,builder(Function)seams that let the middleware tests avoid a real transport, and matched en/zh docs.
Automated review by github-manager-bot
| } | ||
| } | ||
| if (shortlist.isEmpty()) { | ||
| return Mono.just(Set.of()); |
There was a problem hiding this comment.
An empty selection silently strips every optional tool. Set.of() here flows into filteredTools(...), so the model is left with only the three alwaysIncludeTools. The same happens when JevSelectionSupport.selectedNames() returns List.of() because answer.confidence() < confidenceThreshold (support helper, line ~94) — i.e. "Jev is unsure" produces maximal filtering, which is the opposite of the failOpen intent used on the error path just above.
Suggest treating "nothing selected" as a no-op (pass input.tools() through unchanged), or keeping the top-K by probability, and add a test for the below-threshold case so the semantics are pinned.
| public final class JevToolSelectionMiddleware implements MiddlewareBase { | ||
|
|
||
| public static final Set<String> DEFAULT_ALWAYS_INCLUDE_TOOLS = | ||
| Set.of("load_skill_through_path", "reset_tools", "generate_response"); |
There was a problem hiding this comment.
These three names are string literals owned by other modules (generate_response is ReActAgent.STRUCTURED_OUTPUT_TOOL_NAME, load_skill_through_path comes from SkillToolFactory). A rename upstream degrades silently: the tool is no longer force-included, so structured output / skill loading can disappear from the schema mid-conversation with no error.
Could you reference the public constants where they exist, and log (or assert) once at build time when an always-include name is never seen in an incoming schema list?
| return next.apply(input); | ||
| } | ||
|
|
||
| return selectTools(JevSelectionSupport.messagesState(input.messages()), optionalTools) |
There was a problem hiding this comment.
Latency budget on the interactive path. Each reasoning step now issues at least one systemOne() call, two when optionalTools > MAX_CANDIDATES_PER_CHOICE (partition pass + rerank pass), and with JevRetryPolicy.defaults() (2 retries, backoff up to initialBackoff * 8) a single call can take roughly 3 x 5s + 0.5s + 4s ~= 19.5s, so the rerank path is ~39s in the worst case before failOpen kicks in.
Two suggestions:
- add an overall deadline around
retryWhen(outer.timeout(totalBudget)), since the per-attempttimeoutis restarted on each retry; - memoize per turn, keyed on (schema-list identity + latest user text), so consecutive ReAct iterations don't repeat an identical selection call.
| if (messages == null || messages.isEmpty()) { | ||
| return Map.of(); | ||
| } | ||
| return Map.of("messages", List.copyOf(messages)); |
There was a problem hiding this comment.
Raw Msg objects are handed to JevClient.MAPPER, which is configured with PropertyNamingStrategies.SNAKE_CASE. Non-annotated getters on Msg / ContentBlock therefore serialize differently here than everywhere else in the framework (explicit @JsonProperty names are kept, but e.g. msgId-style accessors become msg_id), so the endpoint receives a shape that is not the canonical AgentScope message JSON.
Consider (a) serializing caller-supplied state with a naming-strategy-free copy (MAPPER.copy().setPropertyNamingStrategy(null)) so the snake_case policy only applies to the Jev DTO envelope, and (b) projecting to role + text instead of shipping whole message graphs — that also bounds payload size and keeps tool outputs (which can carry credentials/PII) out of a third-party endpoint. The docs do disclose the full-state behaviour, which is great; an explicit data-egress callout plus a redaction hook would make it safer.
| if (result.answers() == null) { | ||
| throw new JevException("System One response answers must not be null"); | ||
| } | ||
| if (result.usage() == null) { |
There was a problem hiding this comment.
usage is a reporting/billing field, but here a null makes the whole call fail with a non-retryable JevException — the validated answers are simply thrown away, and under the default failOpen=true the middlewares degrade to "Jev never selects" with no signal in the logs. Unless the API contract guarantees usage on every response, I'd treat it as optional.
| requireProbability(id, entry.getValue()); | ||
| sum += entry.getValue(); | ||
| } | ||
| if (Math.abs(sum - 1) > PROBABILITY_SUM_TOLERANCE) { |
There was a problem hiding this comment.
Absolute 1e-6 tolerance over a distribution of up to 255 values: if the provider rounds each probability (e.g. to 6 significant digits, or normalizes after rounding), the sum can legitimately land at 0.99999x and every response gets rejected as non-retryable. A tolerance that scales with the number of options (1e-6 * probabilities.size(), or a relative comparison) or normalizing before comparison would be more robust. Worth a test with a 200+ option criteria map.
|
|
||
| /** Blocking convenience wrapper for {@link #systemOne(SystemOneRequest)}. */ | ||
| public SystemOneResult systemOneBlocking(SystemOneRequest request) { | ||
| return systemOne(request).block(); |
There was a problem hiding this comment.
block() throws IllegalStateException when invoked on a thread marked NonBlocking (reactor event loop), which is easy to hit from within agent/hook code. Either guard with Schedulers.isInNonBlockingThread() and fail with an actionable message, or note the constraint in the javadoc alongside the blocking warning.
| } | ||
| for (AgentSkillRepository repository : repositories) { | ||
| try { | ||
| List<AgentSkill> repositorySkills = repository.getAllSkills(); |
There was a problem hiding this comment.
visibleSkills() calls repository.getAllSkills() on every agent invocation, so a remote-backed repository (Nacos skill marketplace, file store, ...) turns into an extra blocking I/O fan-out per turn, on top of the Jev round trip. Since the skill list is effectively stable within a session, a short-lived cache (or Mono.fromCallable(...).subscribeOn(boundedElastic()) if the repository can block) would keep this off the hot path.
| } | ||
| } | ||
| } | ||
| } catch (RuntimeException ignored) { |
There was a problem hiding this comment.
Failing open is the right call, but the exception is dropped with no logger in the class at all, so a permanently broken repository looks exactly like "no skills configured". A one-line log.debug("skill repository {} failed", repository, ignored) (or a warn with throttling) would save the next reader a lot of guessing.
| "v2/en/integration/ecosystem/index", | ||
| "v2/en/integration/ecosystem/overview", | ||
| "v2/en/integration/ecosystem/chat-completions-web", | ||
| "v2/en/integration/ecosystem/jev", |
There was a problem hiding this comment.
The new page is registered in docs.json, but docs/v2/en/integration/overview.md and docs/v2/zh/integration/overview.md (Ecosystem section, ~line 109) still list only chat-completions-web / studio / training, so Jev is not reachable from the integration index. Please add a link in both languages — every other extension is listed there.
Summary
This PR adds a dedicated Jev extension for TypeSafe System One (https://docs.typesafe.ai/introduction).
The new module provides:
Jev is intentionally not registered as a ChatModel provider. It is a decision model, not a chat model, so the extension exposes it as a typed decision primitive for middleware and application logic.
Changes
agentscope-extensions-jevJevClientwith:JevSkillSuggestionMiddlewareJevToolSelectionMiddlewareNotes
TYPESAFE_API_KEYfirst and falls back toJEV_API_KEY.Model,ChatModelBase, orModelProvider.Testing
mvn -pl agentscope-extensions/agentscope-extensions-jev testnpm --prefix docs run check