fix(harness): resolve ask_user conflicts with current main - #3108
fix(harness): resolve ask_user conflicts with current main#3108mikemikimike wants to merge 13 commits into
Conversation
Adds the HITL ask direction: a tool whose checkPermissions() returns PermissionDecision.askUser(...) pauses the run with a new GenerateReason.ASK_USER_ASKING (in every permission mode, including BYPASS) and is never executed; callers render the questions and resume with AskUserResult(s) under Msg.METADATA_ASK_USER_RESULTS. Core: - PermissionBehavior.ASK_USER + PermissionDecision.askUser() - PermissionEngine: ASK_USER short-circuits the bypass-immune tool self-check - ReActAgent: gate routes ASK_USER to a dedicated pending list; actingStream emits RequireUserAskEvent + RequestStopEvent(ASK_USER_ASKING); acting() returns the pausing assistant Msg unchanged; doCallInner accepts and formats AskUserResults into the ask_user tool result on resume - New events: RequireUserAskEvent / UserAskResultEvent (replyId-correlated) - GenerateReason.ASK_USER_ASKING Harness: - Built-in AskUserTool (ask_user, JSON-schema'd questions[]) - HarnessAgent.Builder.enableAskUser() (opt-in, off by default) Docs + examples + tests: - v1 hitl.md / v2 permission-system.md (en+zh) document the ask direction - AskUserHITLExample runnable example - ReActAgentAskUserTest (core) + HarnessAgentAskUserTest (harness) cover pause under BYPASS, batch questions, skip semantics, resume without executing the tool Closes agentscope-ai#2860
… new events - AskUserResult gains @JsonCreator/@JsonProperty so UserAskResultEvent decodes through the RemoteEventCodec AGENT_EVENT passthrough - RemoteEventCodecPassthroughTest covers REQUIRE_USER_ASK / USER_ASK_RESULT - PermissionBehaviorTest expects five behaviors (ASK_USER added)
…ests Default memory hooks schedule background flush/maintenance after each call; disabling them keeps the AskUser harness tests deterministic and free of post-call background threads in the shared surefire JVM.
…7153) HarnessAgentSubagentStreamTest.call_localSubagent_returnsReplyWithoutStreaming fails on the CI runner with 'Failed to close extension context' at @tempdir teardown. The same failure occurred on main's guava-bump CI (no relation to this PR); the test passes locally. No code change in this commit.
- AskUserEventsTest: full-arg constructors, getters, Jackson round-trips and null-safe payloads for RequireUserAskEvent / UserAskResultEvent; @JsonCreator round-trip + edge branches of AskUserResult (empty list, selected-only, text-only, skipped=false, scalar values, multi-entry) - ReActAgentAskUserTest: resume validation error branches (duplicate answer id, unknown/stale tool call id) - HarnessAgentAskUserTest: AskUserTool.callAsync fallback covers the interactive placeholder; AskUserTool fallback text now carries the real metadata key value
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
This is the conflict-resolved successor of #2865 and wires a second HITL direction (model-initiated ask_user) through PermissionBehavior.ASK_USER, two new events, and an opt-in harness builder flag. The structure is good and the new tests are thorough for the default/BYPASS paths, but the interrupt is not actually mode-exhaustive, and the answer channel has a couple of correctness / credential-handling problems I would like resolved before this is merged.
Findings
- [Critical]
PermissionEngine.java:159—ASK_USERnever reaches the engine inEXPLORE/ACCEPT_EDITSmode, becausecheckExploreMode()short-circuits read-only tools toALLOWbeforetool.checkPermissions()runs;ask_userthen executes its fallbackcallAsync()and the question is silently swallowed instead of pausing. - [Warning]
AskUserTool.java:138— thesecretquestion type cannot be honoured: raw answers are written into the model-visible tool result (persisted context/state) and carried verbatim onUserAskResultEvent. - [Warning]
AskUserResult.java:50—Map.copyOfrejects null answer values (contradicting the documented(no answer)/ skip handling informatAnswerValue) and does not preserve question order. - [Warning]
ReActAgent.java:3035— the ask-user early return skipswriteAutoDeniedResults()and never emitsRequireUserConfirmEventforASKcalls in the same batch. - [Warning]
ReActAgent.java:1775— the ASK_USER branch is checked before the confirmation branch and validates ids against allASKINGcalls, so an answer payload can consume a pending permission confirmation. - [Warning]
GenerateReason.java:72— consumers that special-casePERMISSION_ASKING(service-dataplaneSessionTurnRunner, aistioHarnessAgentTaskStarter,AgentProtocolTaskStore, the AG-UI converters,RemoteEventCodec) were not updated; please say what is intentionally out of scope. - [Info]
PermissionBehavior.java:39andReActAgent.java:1973/2017— rule registration silently ignoresASK_USER; null-id handling inisInAny; answered tool calls keepASKINGstate; import and@SuppressWarningsnits; the title would read better asfeat(...).
Suggestions
- Make the interrupt genuinely mode-exhaustive: evaluate
tool.checkPermissions()before the explore/accept-edits read-only shortcut (or excludeASK_USERfrom that shortcut), and add aPermissionModex pause-kind matrix test — today only default and BYPASS are asserted. - For
secretanswers, resolve the question type from the pausedToolUseBlockinput (the tool call still carriesquestions[]) and mask the value both in the formatted tool result and inUserAskResultEvent; also dropanswersfromAskUserResult.toString(). If masking is out of scope here, removesecretfrom the enum for now rather than advertise a guarantee the runtime breaks. - Prefer
Collections.unmodifiableMap(new LinkedHashMap<>(answers))overMap.copyOfso skipped/null answers render instead of throwing, and the answer text keeps the model question order. - Reconcile the mixed-batch case: write auto-denied results before returning, and either emit
RequireUserConfirmEventalongsideRequireUserAskEventor document that a batch may not mix the two pause kinds. Keying the persisted pending state by pause kind (for example also persisting the asked tool-call ids next to the reply id) would let each resume path validate only its own payload. - The description reports about 1,038 harness tests passing but the full reactor build was cut short by dependency-download interruptions. Could you confirm a clean run of the
agentscope-coretests plus the extensions that switch onGenerateReason, and note theMarketplaceStagerOrphanGcTestfailure separately so reviewers can see it is pre-existing and environment-specific?
Cross-repo Note
RequireUserAskEvent / UserAskResultEvent are additive entries in the AgentEvent @JsonSubTypes registry: a peer running an older agentscope-core (studio or AG-UI frontends, distribution nodes during a rolling upgrade) cannot decode payloads carrying the new names. Worth a compatibility note in the docs page added here, since the same text ships to both docs/v1 and docs/v2.
Automated review by github-manager-bot
| return toolCheckPermissions(tool, input) | ||
| .flatMap( | ||
| toolDecision -> { | ||
| if (toolDecision.getBehavior() == PermissionBehavior.ASK_USER) { |
There was a problem hiding this comment.
ASK_USER is short-circuited here, but this flatMap only sees the decision when toolCheckPermissions() actually reaches tool.checkPermissions(). In EXPLORE and ACCEPT_EDITS it does not: toolCheckPermissions() first calls checkExploreMode(tool), and because AskUserTool is built with .readOnly(true) that helper returns ALLOW immediately, so the tool's own checkPermissions() is never invoked. Result: in those two modes ask_user does not pause at all — callAsync() runs and the model gets the fallback text ("This tool is handled interactively by the host application..."), so the question is silently swallowed. The javadoc on AskUserTool / PermissionDecision.askUser() claims the interrupt happens "in every PermissionMode, including BYPASS"; BYPASS is indeed covered (the bypass fallback lives in continueAfterToolCheck), but EXPLORE/ACCEPT_EDITS are not. Suggested fix: consult the tool's own decision before the explore-mode shortcut (or let checkExploreMode skip tools whose checkPermissions may return ASK_USER), and add a mode matrix test — the new tests only cover the default and BYPASS paths.
| "default", | ||
| "single", | ||
| "description", | ||
| "single=choose one option; multiple=choose any number of options; " |
There was a problem hiding this comment.
The secret question type advertises "the host must not persist or log the raw value", but the framework itself cannot honour that. validateAndAcceptAskUserResults() renders the answers through AskUserResult.formatAnswers() into a ToolResultBlock that is appended to the agent context — i.e. persisted into the session state store and re-sent to the model on every following iteration — and UserAskResultEvent carries the same raw AskUserResult objects to every event consumer (sub-agent forwarding, AG-UI, tracing). Handing a password/API key through this path therefore persists it in state and streams it in events. Either drop secret from the enum until the framework can redact it, or have the resume path resolve the question type from target.getInput() (the tool call still carries the questions payload) and write a masked placeholder (e.g. q_1: ***) for secret answers while AskUserResult.toString() also stops printing raw answers.
| throw new IllegalArgumentException("AskUserResult.toolCallId must not be empty"); | ||
| } | ||
| this.toolCallId = toolCallId; | ||
| this.answers = answers != null ? java.util.Map.copyOf(answers) : java.util.Map.of(); |
There was a problem hiding this comment.
Map.copyOf has two behaviours that fight the rest of this class: (1) it throws NullPointerException for null values, while formatAnswerValue explicitly supports null (renders "(no answer)") and the javadoc describes skipped/unanswered questions — so a legitimate new AskUserResult(id, Map-of-q1-to-null) fails in the constructor with a stack trace instead of being formatted; (2) it does not preserve iteration order, so for multi-question calls the model-visible text (q_1: ...\nq_2: ...) comes out in an arbitrary order per construction, which is a needless source of non-deterministic prompts (and brittle assertions in tests). Collections.unmodifiableMap(new LinkedHashMap<>(answers)) (with nulls either filtered or tolerated consistently) fixes both.
| // correlation, then signal stop via RequestStopEvent. The agent's | ||
| // acting() will set GenerateReason to ASK_USER_ASKING and return; | ||
| // the tool is never executed. | ||
| if (!pendingAskUser.isEmpty()) { |
There was a problem hiding this comment.
This early return runs before the permission-HITL branch, so a batch that contains both an ask_user call and calls gated by ASK/deny rules loses information: RequireUserConfirmEvent is never emitted and writeAutoDeniedResults(toolCalls, autoDenied) is skipped, even though updateToolCallStates already marked those calls ASKING above. The host only sees RequireUserAskEvent, so it is never told about the confirmations it will have to answer afterwards (and the denied calls keep a dangling tool_use with no tool_result until a later round). Please either surface both pauses in one round (emit RequireUserConfirmEvent alongside RequireUserAskEvent, and write the auto-denied results before returning) or explicitly reject/document mixed batches.
| // on the assistant message when the interrupt was emitted; a resume payload that | ||
| // carries AskUserResults is honored regardless of that metadata. | ||
| List<ToolUseBlock> asking = askingToolCalls(); | ||
| if (isAskUserPaused() || hasAskUserResults(msgs)) { |
There was a problem hiding this comment.
isAskUserPaused() || hasAskUserResults(msgs) is evaluated before the asking.isEmpty() confirmation branch, and asking mixes permission-ASK calls with ask_user calls (both are ToolCallState.ASKING). Two consequences: (a) after a mixed pause, a caller that legitimately supplies ConfirmResults gets an IllegalStateException from validateAndAcceptAskUserResults demanding AskUserResults — the run cannot be resumed with confirmations until the questions are answered, which is the reverse of the order the host was told about in the previous finding; (b) an AskUserResult whose id belongs to a permission-ASK call is accepted (its id is in expectedIds), so a confirmation can be consumed as an "answer" and the tool never runs. Consider keying the pending state by pause kind (e.g. persist the ask_user tool-call ids next to the reply id) and validating answer ids against that set, so each resume path only accepts its own payload.
| * The caller resumes by issuing a second {@code agent.call(...)} whose message carries | ||
| * {@code List<AskUserResult>} under {@code Msg.METADATA_ASK_USER_RESULTS}. | ||
| */ | ||
| ASK_USER_ASKING, |
There was a problem hiding this comment.
Adding a pause reason to core is a cross-module change, but the existing special-cases of PERMISSION_ASKING were not updated for ASK_USER_ASKING, so those consumers will treat an ask-user pause as an ordinary turn completion: agentscope-service/.../managed/SessionTurnRunner.java (isCorePermissionAsking, ~line 767 — the data plane will finish the turn and never surface the pending questions), agentscope-extensions-aistio/.../HarnessAgentTaskStarter.java:365 (auto-approval loop exits with the question unanswered), and agentscope-extensions-agent-protocol/.../AgentProtocolTaskStore.java:268. Same for the AG-UI PermissionConfirmEventConverter / SubagentEventConverter and RemoteEventCodec.toRemoteEvent, which have no branch for RequireUserAskEvent/UserAskResultEvent (they fall back to the generic AGENT_EVENT payload, which older peers cannot decode since the two new @JsonSubTypes names are unknown to them). Please state in the PR description which bridges are intentionally out of scope for this follow-up of #2865, and prefer a default-branch/Unknown handling for the wire compatibility.
| + ". Expected: " | ||
| + expectedIds); | ||
| } | ||
| ToolUseBlock target = questionToolCall(asking, toolCallId); |
There was a problem hiding this comment.
Nit: unlike applyConfirmResults, this path never moves the answered ToolUseBlock out of ToolCallState.ASKING (no updateToolCallStates(...) / block replacement), so the persisted assistant message keeps saying "asking" for a call that already has a result. Nothing depends on it today, but it makes the context harder to reason about after a state reload and inconsistent with the permission path. Setting the answered calls to ALLOWED (or FINISHED) right after writing the answer message would keep both paths symmetrical.
| ALLOW("allow"), | ||
| DENY("deny"), | ||
| ASK("ask"), | ||
| ASK_USER("ask_user"), |
There was a problem hiding this comment.
The javadoc says ASK_USER "is not registerable as a rule", but nothing enforces it: PermissionEngine.addRule()'s switch (lines ~96-105) has cases for ALLOW/DENY/ASK/PASSTHROUGH and no default, so an ASK_USER rule is accepted, stored nowhere and silently ignored — the worst failure mode for a permission config. Adding case ASK_USER -> throw new IllegalArgumentException(...) (and the same in PermissionRule's compact constructor) would make the contract real. Also worth a note in the changelog: PermissionBehavior.fromString("ask_user") and the two new AgentEvent subtypes are additive, so state/config written by a newer build is not readable by an older one.
| * iteration reads the user's answers without executing the tool. The correlated | ||
| * {@link UserAskResultEvent} is emitted for streaming consumers. | ||
| */ | ||
| private void validateAndAcceptAskUserResults(List<Msg> msgs, List<ToolUseBlock> asking) { |
There was a problem hiding this comment.
Minor style points in this change set: isInAny dereferences id.equals(...) where id is tc.getId(), while the surrounding code deliberately treats tool-call ids as possibly null (filter(Objects::nonNull) when building expectedIds) — Objects.equals(p.getId(), id) would be null-safe; extractAskUserResults keeps an @SuppressWarnings("unchecked") that no longer suppresses anything (the method uses pattern matching); AskUserResult and UserAskResultEvent use fully-qualified java.util.Map/java.util.List instead of imports, which differs from the rest of io.agentscope.core.event. And since this PR adds public API (PermissionBehavior.ASK_USER, two events, two Msg metadata keys, PermissionDecision.askUser), fix(harness): ... understates it — feat(core,harness): add ask_user HITL direction reads truer for the changelog.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of c448d95e ("harden ask_user HITL flow"). The new commit resolves most of my previous round: ASK_USER is now evaluated before the EXPLORE / ACCEPT_EDITS read-only shortcut (with tests for both modes), AskUserResult keeps insertion order and tolerates null answers instead of Map.copyOf, answered calls are moved out of ASKING into FINISHED, auto-denied results are written before the ask-user pause, and secret answers are now masked in both the model-visible tool result and UserAskResultEvent. Thanks — that was the right shape of fix.
What keeps this at COMMENT rather than approve: the redaction added here is fail-open against answer-key mismatch (the leak class it targets), the pause-kind detection now depends entirely on persisted metadata that a reloaded or compacted session may not carry (a legitimate resume can become unrecoverable), and the mixed-batch case aborts the run instead of degrading. Separately, one item from the previous round is still open and out of this diff's reach:
- Cross-module
GenerateReason.ASK_USER_ASKINGconsumers still not updated. At this HEAD,agentscope-service/service-dataplane/.../managed/SessionTurnRunner.java:767still tests only== GenerateReason.PERMISSION_ASKING, and neitheragentscope-extensions-aistio/.../HarnessAgentTaskStarter.javanoragentscope-extensions-agent-protocol/.../AgentProtocolTaskStore.javamentionsASK_USERat all, so a managed/aistio/agent-protocol run that pauses to ask a question is reported as an ordinary completed turn and the questions are never surfaced to the host.RemoteEventCodecPassthroughTestdoes show the two new events passing through the sub-agent codec, so that part is covered. If the service and extension paths are intentionally out of scope for this PR, please say so in the description and file a follow-up issue so the gap is tracked rather than silent.
Findings
See the inline comments: 3 warnings (ReActAgent.java:2062 fail-open secret redaction, ReActAgent.java:1782 pause kind derived only from reply-id metadata, ReActAgent.java:3426 mixed-batch hard abort) and 2 infos (PermissionEngine.java:225 mode-shortcut ordering side effects, AskUserResult.java:114 redaction is indistinguishable from a literal [REDACTED] answer).
Suggestions
- For a tool call that declares any
type: "secret"question, mask every answer key that is not a declared non-secret id, and log a warning when a declared secret id matched no answer key. - Derive the pause kind from the pending
ASKINGtool calls (name resolves to a tool whose self-check returnedASK_USER) and keep the reply-id metadata as correlation only, so a state reload or compaction that loses the metadata cannot strand a session. - Degrade on a mixed
ASK+ASK_USERbatch instead of failing the turn: keep one pause pending, or synthesise an error tool result for the offending subset so the model can re-issue it. - Worth one line in both new docs sections:
DONT_ASKdoes not auto-decline anASK_USERpause.
Notes
CI on this HEAD: build (ubuntu-latest), build (windows-latest), validate, Check License, Check Module Sync, codecov/patch all pass, license/cla is signed, merge state is MERGEABLE/BLOCKED. Tests added are proportionate and cover the redaction, ordering, and mode-exhaustiveness paths, so the remaining items above are hardening/robustness rather than regressions.
Automated review by github-manager-bot
| } | ||
|
|
||
| /** Returns the ids of secret questions in one ask_user tool call. */ | ||
| private Set<String> secretQuestionIds(ToolUseBlock toolCall) { |
There was a problem hiding this comment.
[Warning] secret redaction is fail-open when the host keys answers differently. formatAnswers / redactedFor mask only the entries whose key is literally one of the declared secret question ids, so a host that answers with the question text (or a per-question id it invented because the model omitted id) writes the raw API key / password into the model-visible ToolResultBlock, which is then persisted into the session state store — exactly the leak this hardening commit set out to close, and the failure is silent. Please invert the policy for a tool call that contains at least one type: "secret" question: start from the declared ids and mask everything that is not a declared non-secret id (whitelist instead of blacklist), so an unknown key is redacted rather than rendered verbatim. A cheap companion guard is to log at WARN when secretIds is non-empty but none of them matched an answer key, so hosts can discover the mismatch instead of leaking.
| } | ||
| if (hasAskUserResults(msgs)) { | ||
| throw new IllegalStateException( | ||
| "AskUserResult was supplied, but no ASK_USER pause is pending. " |
There was a problem hiding this comment.
[Warning] The pause kind is now inferred solely from persisted reply-id metadata, which can drop a legitimate resume. isAskUserPaused() returns !resolvePendingRequestReplyId(METADATA_ASK_REQUEST_REPLY_ID).isEmpty(), and that helper reads the metadata of findLastAssistantMsg(). If that metadata is absent — state written before this build, a session reloaded through AgentStateStore, or a harness context-compaction pass that replaced/trimmed the last assistant message — a host that correctly sends List<AskUserResult> after a RequireUserAskEvent now hits this IllegalStateException and can never resume the run, while the ASKING ask_user calls stay pending forever. The previous isAskUserPaused() || hasAskUserResults(msgs) was loose but never stranded a session. Suggest deriving the pause kind from the pending blocks themselves (asking.stream().anyMatch(t -> "ask_user".equals(t.getName()) && secret-ish shape) — i.e. treat a call whose name resolves to a tool that returned ASK_USER from checkPermissions as an ASK_USER pause) and keeping the reply-id metadata only as a correlation hint, or at minimum auto-healing the missing metadata when every pending ASKING id belongs to an ask_user call.
| return new PermissionGate(pending, denied); | ||
| if (!pending.isEmpty() && !pendingAskUser.isEmpty()) { | ||
| throw new IllegalStateException( | ||
| "A single model tool batch cannot mix permission " |
There was a problem hiding this comment.
[Warning] Failing the entire call for a model-controlled batch shape. The mixing check runs inside evaluatePermissions(...).map(...), so the exception becomes an onError on the whole turn: no RequireUserAskEvent, no RequireUserConfirmEvent, no denied results written, and updateToolCallStates(...) in actingStream never runs — the host just sees a failed call for something the model chose to emit. Batching is model-controlled, so this path is reachable in normal use of ask_user next to a gated tool, and DONT_ASK/BYPASS hosts have no way to clear it. Documenting the constraint in the PermissionGate javadoc is good, but please also degrade instead of aborting: e.g. keep the ASK calls pending for the next round and emit RequireUserAskEvent for the ASK_USER ones (or synthesise an error ToolResultBlock for the offending subset so the model can re-issue them separately). Either way the run should stay resumable.
| if (decision.getBehavior() == PermissionBehavior.ASK_USER) { | ||
| return Mono.just(decision); | ||
| } | ||
| PermissionDecision modeDecision = checkExploreMode(tool); |
There was a problem hiding this comment.
[Info] The mode shortcut now always awaits the tool self-check. Correct fix for the EXPLORE / ACCEPT_EDITS bypass, and the new PermissionEngineTest cases cover both directions. Two consequences worth a javadoc note: (1) a tool's checkPermissions() is now invoked on every call in read-only modes even though checkExploreMode will usually override its answer, so self-checks that are async or side-effecting (remote policy lookups, counters, audit logs) now run in EXPLORE; (2) in ACCEPT_EDITS a read-only tool's own DENY is still overridden to ALLOW by the mode decision because the mode is consulted after the tool. Both are pre-existing semantics, just now on a path that is easier to change — happy to see them called out in the method comment so the next reader does not have to re-derive the ordering.
| * @param secretQuestionIds question ids whose values must be redacted | ||
| * @return this result when no redaction is needed, otherwise a redacted copy | ||
| */ | ||
| public AskUserResult redactedFor(Set<String> secretQuestionIds) { |
There was a problem hiding this comment.
[Info] Consider exposing which answers were redacted. redactedFor replaces secret values with the literal "[REDACTED]", so a UI or audit consumer of UserAskResultEvent cannot distinguish "the user typed [REDACTED]" from "this value was masked", and it loses the count of answered questions. Adding something like getRedactedQuestionIds() (or a Map<String,Object> answers, Set<String> redacted pair) on the event side keeps the value out of the stream while letting hosts render answered (hidden) correctly. Also worth stating in the docs/v1 + docs/v2 sections added here that ASK_USER pauses are not auto-declined by DONT_ASK, since that mode is what unattended hosts reach for — the docs currently only say "do not register the tool".
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of c415cd30 ("harden ask_user pause handling"). All three blocking items from my previous round are addressed, and addressed well:
- Fail-closed secret redaction.
formatAnswers(answers, nonSecretQuestionIds, hasSecretQuestion)/redactedFor(...)now mask every key that is not a declared non-secret id whenever the tool call declared a secret question, andwarnIfSecretAnswersDoNotMatchlogs when a declared secret id matched nothing. The newsecretAnswerWithMismatchedKeyIsRedactedFromModelContextAndEventsandfailClosedRedactionMasksUnknownAnswerKeysWhenSecretQuestionExiststests pin exactly the leak class I described. - Pause kind no longer depends on reply-id metadata alone.
classifyAskingToolCallsderives it from the per-tool-callPermissionBehaviornow persisted viaupdateToolCallStates(..., behaviorUpdates), with the built-inask_userinput shape and a re-check throughevaluateOneas recovery for states written before the marker existed.resumeRecoversWhenAskUserReplyMetadataIsLostandresumeRecoversCustomAskUserWhenAllPauseMetadataIsLostcover both. That removes the "stranded session" failure mode. - Mixed
ASK+ASK_USERbatches degrade instead of aborting. TheIllegalStateExceptioninevaluatePermissionsis gone and both pause kinds are surfaced, which is the behaviour I asked for. One consequence of the new branch still needs a decision — see the inline[Warning]on the stop reason.
Why this is still COMMENT rather than approve
build (ubuntu-latest) is failing on this HEAD and build (windows-latest) was cancelled with it:
io.agentscope.harness.agent.AguiPermissionResumeTest.approvedToolResultSurvivesNewStreamContext(boolean, boolean)[2]
RunError[threadId=resume-thread, runId=second,
message=Thread already has an active run; wait for run first to finish
before starting another run on the same thread,
code=AGUI_INTERRUPT_CONTRACT_ERROR] ==> expected: <true> but was: <false>
Tests run: 1045, Failures: 1, Errors: 0, Skipped: 9
The failing assertion is the AG-UI active-run marker race that #3109 fixes: the rejected run is the officialResume variant, and the message (Thread already has an active run; wait for run first to finish) is exactly what #3109's doOnComplete/doOnError change removes. Nothing in this diff touches AguiRequestProcessor, and this branch does not contain that fix. So this reads as the pre-existing intermittent race rather than a regression you introduced — but #3109 is still open, so it will keep being red until one of them lands. Two options, either is fine:
- rebase once #3109 lands, or
- if a maintainer would rather not couple the two PRs, note the known-flake link in the description (and re-run once) so the red check is not read as a regression in this PR. If a re-run fails on the same variant, that stops being the flake story and I'd want to look again.
Findings
See the inline comments: 1 warning (ReActAgent.java:3263-3268 — a mixed pause reports only ASK_USER_ASKING, so the confirmation half is invisible to the PERMISSION_ASKING consumers) and 2 infos (AskUserResult.java:91 — the retained fail-open overload, ReActAgent.java:4382 — framework key inside provider-facing ToolUseBlock.metadata).
The cross-module GenerateReason consumers I raised last round are still unaddressed (SessionTurnRunner.java:767, HarnessAgentTaskStarter.java:365, AgentProtocolTaskStore.java:268); the mixed-pause branch above makes that gap reachable from a new state, so it is now worth resolving here rather than in a follow-up.
Suggestions
- Derive the stop reason from the set of outstanding pause kinds (or emit
PERMISSION_ASKINGwhenpendingis non-empty) and update the three consumers, or add an explicit combined reason. - Consider
@Deprecatedon the two-argumentformatAnswers/redactedForoverloads so hosts cannot silently keep the fail-open behaviour. - Worth one sentence in the docs sections added here:
DONT_ASKdoes not auto-decline anASK_USERpause, and a mixed batch now pauses for both kinds.
Automated review by github-manager-bot
| return Flux.<AgentEvent>just( | ||
| new RequireUserAskEvent(replyId, pendingAskUser), | ||
| new RequireUserConfirmEvent(replyId, pending), | ||
| new RequestStopEvent( | ||
| "ask user and permission confirmation", | ||
| GenerateReason.ASK_USER_ASKING)); |
There was a problem hiding this comment.
[Warning] A mixed pause is reported as ASK_USER_ASKING only, so the confirmation half is invisible to the PERMISSION_ASKING consumers. This branch emits both RequireUserAskEvent and RequireUserConfirmEvent but stops with GenerateReason.ASK_USER_ASKING; the three places that special-case PERMISSION_ASKING will not see a permission pause at all:
agentscope-service/service-dataplane/.../managed/SessionTurnRunner.java:767(isCorePermissionAsking→ durable confirmation not persisted, and the guard at line 611 never fires)agentscope-extensions/agentscope-extensions-aistio/.../adapter/HarnessAgentTaskStarter.java:365agentscope-extensions/agentscope-extensions-protocol/agentscope-extensions-agent-protocol/.../AgentProtocolTaskStore.java:268
Before this commit a mixed batch could not exist (it threw), so this is a new state on the wire rather than a pre-existing gap: a managed/aistio/agent-protocol run that pauses for both will surface the questions, drop the pending confirmation, and the caller has no reason code telling it a second pause kind is outstanding. Either pick the reason from the set of outstanding pause kinds (or add PERMISSION_ASKING when pending is non-empty), or add an explicit combined reason and update those three consumers in this PR so the gap is not silent.
This is the same cross-module item I flagged last round; the GenerateReason.ASK_USER_ASKING javadoc still only documents the question case.
| * @param secretQuestionIds question ids whose values must not be exposed | ||
| * @return a stable, human-readable rendering of the answers with secret values redacted | ||
| */ | ||
| public static String formatAnswers(Map<String, Object> answers, Set<String> secretQuestionIds) { |
There was a problem hiding this comment.
[Info] The fail-closed overload is the right fix, and the new failClosedRedactionMasksUnknownAnswerKeysWhenSecretQuestionExists test pins it. One follow-up: this two-argument form is still public and keeps the old fail-open semantics (Set = ids to mask), so a host that migrates by accident gets the weak behaviour with no signal. Consider @Deprecated on it (pointing at the three-argument form) or documenting the parameter name difference loudly — the two overloads take the same Set<String> type with opposite meaning, which is easy to pass backwards at a call site.
| } else { | ||
| Map<String, Object> metadata = new HashMap<>(t.getMetadata()); | ||
| metadata.put( | ||
| ToolUseBlock.METADATA_PERMISSION_BEHAVIOR, behavior.name()); |
There was a problem hiding this comment.
[Info] Nice — persisting the resolved behavior per tool call is what makes the pause kind survive compaction/reload instead of depending on reply-id metadata, and it removes the failure mode I raised last round. Two small notes on putting a framework key into ToolUseBlock.metadata:
- The field was previously documented as provider-specific, and provider converters walk it (Gemini
thoughtSignature, OpenAIreasoningDetail). Nothing bulk-copies it into an outbound payload today, so this is safe now, but the javadoc rename to "provider/framework-specific" is doing real load-bearing work — worth calling out in the PR description so future formatters do not forward unknown keys verbatim. - The key lands in persisted
AgentState/ session JSON, so it is effectively wire-visible state going forward.permissionBehavior()already tolerates a garbage value (unknown string → recovery path), which is the right posture for old-vs-new state.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
The new commit 949db2e5 ("fix(core,adapters): preserve mixed HITL pauses") introduces GenerateReason.PERMISSION_AND_ASK_USER_ASKING to represent model responses containing both ask_user and permission-gated tool calls. The fix correctly updates all consumers across core, harness, aistio, agent-protocol, and service modules. The previously reported "mixed pause reported as ASK_USER_ASKING only, confirmation half invisible" issue is now resolved.
CI
Build (ubuntu-latest) FAILS: HarnessAgentAskUserTest.askUserPausesEvenUnderBypassMode errors with JUnitException: Failed to close extension context, root cause IOException: Failed to delete temp directory ... s-bypass (DirectoryNotEmptyException).
Root cause: PR-owned test code. HarnessAgentAskUserTest wires .workspace(path) to the JUnit @TempDir but has no @AfterEach and never closes the created agents, so session dirs (e.g. s-bypass/) still hold transcript/mirror files and background executors may still be I/O-active when JUnit deletes the temp dir. This is not the upstream @TempDir flake referenced in a0d565a/75c73811 — the failing class is introduced by this PR.
Fix: Add @AfterEach void closeAgent() { agent.close(); } or call .disableTranscript() in the builder at line 109.
Findings
- HarnessAgentAskUserTest.java:109 (critical) — New test never closes the agents it creates; JUnit
@TempDirteardown fails (Failed to delete temp directory ... s-bypass), which is what redsbuild (ubuntu-latest). - AgentProtocolTaskStore.java:269 (warning) — Mixed pause (
PERMISSION_AND_ASK_USER_ASKING) silently drops the ask_user subset. The adapter handles only permission confirmations and discards ask_user calls, violating the documented contract that adapters "must preserve the other subset." - SessionTurnRunner.java:764 (info) —
isCorePermissionAskingnow returns true for mixed pauses, triggering an exception with a misleading message ("PERMISSION_ASKING" instead of "PERMISSION_AND_ASK_USER_ASKING"). - GenerateReason.java:82 (info) — New enum value breaks backward compatibility for strict deserializers. Ensure lenient enum parsing in all event consumers.
Automated review by github-manager-bot
| .stateStore(new InMemoryAgentStateStore()) | ||
| .disableCompaction() | ||
| .disableMemoryHooks() | ||
| .enableAskUser() |
There was a problem hiding this comment.
CI failure (build ubuntu-latest): the new test never releases the agents it creates, breaking @TempDir teardown.
The test class has no @AfterEach and no agent.close() call anywhere (verified: grep finds zero), while build(...) wires .workspace(...) to the JUnit @TempDir. After the run, session directories (e.g. s-bypass/) still contain transcript/mirror files and background executors (session mirror / maintenance scheduler) may still be alive, so JUnit fails with IOException: Failed to delete temp directory ... <root>, s-bypass -> JUnitException: Failed to close extension context (see the failed build on this head).
Fix: keep the created agents in fields and close() them in @AfterEach (or close each agent in a try/finally per test), so mirror I/O is quiesced before the temp dir is deleted. This looks PR-owned rather than the upstream @TempDir flake referenced in a0d565a/75c73811: the failing class and its workspace layout are introduced by this PR.
| Msg reply = resultRef.get(); | ||
| if (reply != null | ||
| && reply.getGenerateReason() == GenerateReason.PERMISSION_ASKING | ||
| && isPermissionApprovalPause(reply.getGenerateReason()) |
There was a problem hiding this comment.
Mixed pause silently drops the ask_user subset.
When the agent returns GenerateReason.PERMISSION_AND_ASK_USER_ASKING, this adapter handles only the permission confirmation calls (via extractPendingConfirms at line 271) and sends a REQUIRE_CONFIRM event. The ask_user subset is never surfaced to the client and is lost when the run resumes with only ConfirmResult metadata. The PR docs state "adapters that support only one pause kind must preserve the other subset rather than treating the turn as complete," but this adapter discards the ask_user calls.
Impact: When permission confirmations are received and the agent resumes, the model will see incomplete state (missing ask_user results). Consider throwing IllegalStateException for mixed pauses (like pure ASK_USER pauses at line 286-289), or persisting the ask_user pending calls alongside permission confirmations.
| @@ -764,7 +764,9 @@ private void persistRemainingThinking( | |||
| static boolean isCorePermissionAsking(AgentEvent event) { | |||
There was a problem hiding this comment.
Error message is misleading for mixed pauses.
isCorePermissionAsking now returns true for PERMISSION_AND_ASK_USER_ASKING, which triggers the CorePermissionConfirmationException at line 610. The exception message says "Core PermissionEngine returned PERMISSION_ASKING..." but for mixed pauses the actual reason is PERMISSION_AND_ASK_USER_ASKING. The message should reflect the actual reason to aid debugging.
Fix: Capture the actual GenerateReason from the AgentResultEvent and include it in the exception message at line 611.
| * {@code AskUserResult}s for question calls. Adapters that only support one pause kind must | ||
| * preserve the other subset instead of treating this reason as a completed turn. | ||
| */ | ||
| PERMISSION_AND_ASK_USER_ASKING, |
There was a problem hiding this comment.
Backward compatibility: new enum value breaks old deserializers.
Adding PERMISSION_AND_ASK_USER_ASKING to the GenerateReason enum is a breaking change for serialized events. Old clients that deserialize this value using GenerateReason.valueOf(String) will throw IllegalArgumentException. While most JSON libraries handle unknown enum values gracefully (returning null or a default), clients that use strict enum parsing will fail.
Mitigation: Ensure all event consumers use lenient enum parsing (e.g., @JsonEnumDefaultValue or try-catch around valueOf). Document this in the release notes.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Commit 54dcaaa1 ("address ask_user review follow-ups") resolves every issue from the previous review: the HarnessAgentAskUserTest agents are now registered and closed in @AfterEach (the ubuntu build that was red on Failed to delete temp directory now passes), AgentProtocolTaskStore refuses a mixed pause with an explicit error instead of silently dropping the ask_user subset, the SessionTurnRunner exception message reports the actual reason, and the new lenient @JsonCreator closes the strict-deserializer compatibility gap with a test. Remaining note is a non-blocking observability suggestion below.
CI
build (ubuntu-latest) passes; build (windows-latest) still pending at review time.
Automated review by github-manager-bot
| * should preserve the raw wire payload or upgrade before handling the turn. | ||
| */ | ||
| @JsonCreator | ||
| public static GenerateReason fromJson(String value) { |
There was a problem hiding this comment.
The silent MODEL_STOP fallback is a reasonable wire-compatibility choice, but for a pause reason it makes an unfinished turn look finished to an older reader. Consider at least a LOG.warn (or a one-line comment pointing to a metric) in the IllegalArgumentException branch so a fallback that swallows a pending HITL/ask_user pause is observable in production rather than only in the javadoc.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review after 40a01b62. The commit itself is a two-line comment on the GenerateReason.fromJson fallback — the code path is unchanged, so my previous review of 54dcaaa1 still stands. One blocking-ish item though: CI is red on this head (build (windows-latest) failed, build (ubuntu-latest) was cancelled as a consequence), and it failed in AguiPermissionResumeTest, which passes on main. Since the only delta since the green 54dcaaa1 run is a comment, this needs either a re-run or a real fix before this can be merged — I have downgraded my previous approval to a comment so the green checkmark does not outlive the red CI.
Findings
- [Warning]
agentscope-core/src/main/java/io/agentscope/core/message/GenerateReason.java:125— the comment documents a risk that the code still swallows; consider making the fallback observable.
Next step
Re-run Java CI with Maven on 40a01b62. If AguiPermissionResumeTest.approvedToolResultSurvivesNewStreamContext fails again, the Thread already has an active run error is a genuine interaction with #3100 (already in main) rather than a flake, and the mixed HITL pause bookkeeping in this PR is the first place I would look.
Automated review by github-manager-bot
| return valueOf(value); | ||
| } catch (IllegalArgumentException ignored) { | ||
| // Keep this fallback observable via metrics/logging; it may hide a pending | ||
| // HITL/ASK_USER pause. |
There was a problem hiding this comment.
The comment now documents the risk, but the silent MODEL_STOP fallback is still there: an unknown/typo'd reason string on the wire (for example a serialized ASK_USER pause produced by an older peer) is downgraded to a normal model stop, so a pending HITL pause is dropped instead of surfaced. Since the fallback is already annotated as potentially hiding an ASK_USER pause, could this commit also emit a log.warn (or a counter) with the rejected value? A code comment alone leaves the failure mode invisible in production.
3 participants
Summary
This is a conflict-resolved continuation of PR #2865 for the built-in
ask_userHITL direction. The original PR branch is owned by another fork, so this branch carries the feature onto the current upstreammainfrommikemikimike/agentscope-java.Changes
ASK_USERdecisions before the EXPLORE / ACCEPT_EDITS read-only shortcut, so model-initiated questions pause in every permission mode.secretanswers in model-visible tool results andUserAskResultEvent, including unknown or mismatched answer keys when a secret question is present. Answer insertion order and null answers remain supported, andAskUserResult.toString()does not expose answer contents.ask_usershape or a tool permission self-check when message metadata is missing, so reloaded or compacted sessions can still resume correctly.ASKandASK_USERcalls in one model batch now emit both pause events withGenerateReason.PERMISSION_AND_ASK_USER_ASKINGand resume independently instead of aborting the turn. Answered calls are marked finished, and auto-denied results are persisted before pausing.ASK_USERsubset as ordinary completion.Tests
D:\CI\run-pre-pr.ps1Java 17 targeted Core validation passed forReActAgentAskUserTest,GenerateReasonTest, andPermissionRuleTest.D:\CI\run-pre-pr.ps1Java 17 targeted Agent Protocol validation passed: 4 tests, 0 failures, 0 errors.D:\CI\run-pre-pr.ps1Java 17 targeted Aistio validation passed: 6 tests, 0 failures, 0 errors.D:\CI\run-pre-pr.ps1Java 17 targeted service-dataplane validation passed: 9 tests, 0 failures, 0 errors.D:\CI\run-pre-pr.ps1Java 17 targeted Harness ask-user validation passed: 5 tests, 0 failures, 0 errors, includingaskUserPausesEvenUnderBypassMode.git diff --checkand Spotless checks passed for the follow-up changes.MarketplaceStagerOrphanGcTest.unreadableEntryDoesNotFailStage: a temporary.skills-cachepath raisedNoSuchFileExceptionunder the Docker/POSIX-permission runner. The targeted ask-user test passed in the same runner, and the same Marketplace error was observed before this follow-up.AguiPermissionResumeTestpassed all four parameter combinations. The Ubuntu Harness job ran 1,045 tests with 0 assertion failures and 1 error while closing the JUnit extension context forHarnessAgentAskUserTest.askUserPausesEvenUnderBypassMode; the targeted rerun passed locally, indicating a non-deterministic test-runner failure rather than a reproducible failure in this change. The Windows job was cancelled by fail-fast.Compatibility / Known limitations
ask_userremains opt-in throughHarnessAgent.Builder.enableAskUser(); existing builders retain their behavior.ASK_USERresume protocol to AG-UI, Agent Protocol, Aistio, or managed service. Those bridges now refuse or report unsupported question pauses explicitly rather than treating them as successful completion; a dedicated question-resume transport remains a separate follow-up.ToolUseBlock.metadatais persisted and may be wire-visible provider/framework-specific metadata.agentscope.permissionBehavioris an internal marker used for pause recovery and mixed-batch routing; integrations should preserve it when persisting/resuming and should not treat it as a provider contract.RequireUserAskEventandUserAskResultEventare additive event types. Olderagentscope-corepeers cannot decode payloads carrying the new event names during a rolling upgrade.Issue link
Closes #2860