Conversation
… state ReActAgent.loadOrCreateAgentStateForSlot forwards the caller's permCtx on the fresh-state path, but its legacy-state path called the 3-arg LegacyStateLoader.loadFromLegacySessionWithPresence overload, which passes null and silently downgrades a configured permission mode (e.g. BYPASS) to DEFAULT. This re-introduces the regression fixed in agentscope-ai#2769 / agentscope-ai#2886 after agentscope-ai#2760 rewrote the method. Forward permCtx through the existing 4-arg overload so legacy v1 sessions keep the caller-configured permission context. Re-adds the regression test removed by agentscope-ai#2760. Closes agentscope-ai#2888 Signed-off-by: wylovelyi <wylovelyi@users.noreply.github.com>
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
One-line forwarding of the caller's PermissionContextState into the legacy v1 session load path — the fix itself is correct and well targeted at #2888, and the permCtx overload it calls is already on main. Two things block it right now, both visible in CI: the new test file's license header is malformed (Check License fails) and the new sessionId assertion fails because LegacyStateLoader never sets sessionId/userId on the reconstructed state (build (ubuntu-latest) fails). Fix the header and either forward the slot identity or scope the test down, and this is good to go.
Findings
- [Critical]
agentscope-core/src/test/java/io/agentscope/core/agent/ReActAgentLegacyPermissionContextTest.java:6— license header missing the trailingat, failsCheck License. - [Critical]
...ReActAgentLegacyPermissionContextTest.java:79— fails in CI:LegacyStateLoader.loadFromLegacySessionWithPresencedoes not setsessionId/userId, so the migrated state gets a random id. - [Info]
agentscope-core/src/main/java/io/agentscope/core/ReActAgent.java:440— permission-context forwarding verified correct againstLegacyStateLoader.java:113-133.
Verification seen
license/clais green, so CLA is signed; CI runs on head6958aa38showCheck Licenseandbuild (ubuntu-latest)failing for the two reasons above.- The second test (
existingV2State_winsOverBuilderPermissionContext) is a nice touch — it pins that an existing v2 state is not clobbered by the builder template.
Automated review by github-manager-bot
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License |
There was a problem hiding this comment.
[Critical] The license header is malformed — line 6 is missing the trailing at (You may obtain a copy of the License → should be You may obtain a copy of the License at). This is what makes the Check License job fail on this PR; please copy the exact header from a neighbouring file (e.g. ReActAgentHitlTest.java) or run license-eye header fix locally.
| PermissionMode.BYPASS, | ||
| state.getPermissionContext().getMode(), | ||
| "builder-supplied permission context must survive legacy v1 session loading"); | ||
| assertEquals( |
There was a problem hiding this comment.
[Critical] This assertion fails in CI (build (ubuntu-latest)): expected: <session-legacy-perm> but was: <92536d6e...>. The test is right, the production code is not: LegacyStateLoader.loadFromLegacySessionWithPresence(...) builds the state with AgentState.builder().context(msgs) and never calls .sessionId(...)/.userId(...), so AgentState falls back to newHex() for sessionId. That means the legacy migration path returns a state whose identity does not match the (userId, sessionId) slot it was loaded for — a second regression next to the permission-context one this PR fixes.
Suggested direction (either is fine, but the PR must be green):
- Forward the slot identity here too, e.g. pass
sessionId/userId(or the whole slot) into the legacy loader and set them on the builder — consistent withfreshState(permCtx, agentId, userId, sessionId, ...), which does set them; or - If you prefer to keep this PR to the one-line permission fix, drop these two identity assertions and open a follow-up issue so the
sessionIdloss is tracked separately.
| LegacyStateLoader.LegacyLoadResult legacy = | ||
| LegacyStateLoader.loadFromLegacySessionWithPresence(stateStore, userId, sessionId); | ||
| LegacyStateLoader.loadFromLegacySessionWithPresence( | ||
| stateStore, userId, sessionId, permCtx); |
There was a problem hiding this comment.
[Info] The forwarding itself is correct — the 4-arg overload already applies permCtx (LegacyStateLoader.java:122-125), loadFromLegacySessionWithPresence is only called from this one site, and legacy keys never carry a permission context, so the builder-supplied template is the right fallback. No change needed here; just note that the same reconstruction also drops sessionId/userId (see the inline comment on the new test), which is why the added test currently fails.
2 participants
What
ReActAgent.loadOrCreateAgentStateForSlotnow forwards the caller'sPermissionContextStatethrough the legacy v1 session load path, so sessions that only carry v1 keys (memory_messages/toolkit_activeGroups) keep their configured permission mode (e.g.BYPASS) instead of being silently downgraded toDEFAULT.Why
Closes #2888. This is a regression: #2760 rewrote the method and reverted the legacy-state path to the 3-arg
LegacyStateLoader.loadFromLegacySessionWithPresence(stateStore, userId, sessionId)overload, which passesnullforpermCtx. The 4-arg overload that forwardspermCtxalready exists (added in #2886 / #2769), so only the call site needed fixing — exactly the change #2760 dropped.How
agentscope-core/src/main/java/io/agentscope/core/ReActAgent.java:439— callloadFromLegacySessionWithPresence(stateStore, userId, sessionId, permCtx)instead of the 3-arg overload.ReActAgentLegacyPermissionContextTest(removed by fix(core): propagate agent state load failures #2760) which pins the legacy + permission-context combination. The test triggers the legacy load viagetAgentState()and asserts the builder-suppliedBYPASSmode survives (and that an existing v2 state keeps its ownDEFAULT).Verification
permCtxto the reconstructed state (LegacyStateLoader.java:122-125).loadOrCreateAgentStateForSlotis exercised bygetAgentState()(ReActAgent.java:4407), which the new test drives.Self-review (P0-P3)
main(4-arg overload present). ✅