Skip to content

refactor(core): drop unused role parameter from message #3045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
seuCHENGHQ wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor(core): drop unused role parameter from message #3045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions agentscope-core/src/main/java/io/agentscope/core/message/AssistantMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ public AssistantMessage(List<ContentBlock> blocks) {
private AssistantMessage(
@JsonProperty("id") String id,

@oss-maintainer oss-maintainer Sep 11, 2026

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.

[Warning] Removing this parameter does change deserialization behaviour, because Msg declares @JsonTypeInfo(..., visible = true) — the role value from JSON is actively forwarded to the subtype creator rather than consumed only as the type discriminator.

With the parameter gone, role is no longer bindable by this creator, so it is dropped as an unknown property. That is tolerated only because Msg carries @JsonIgnoreProperties(ignoreUnknown = true) and that annotation is @Inherited. So the code compiles and round-trips today, but the cleanup now depends on an inherited annotation on the base class that nothing in this file enforces: if ignoreUnknown were ever narrowed, deserializing these four subtypes would start failing with UnrecognizedPropertyException.

Two consequences worth locking down with a wire-compat test before merge:

  1. A payload whose role is absent or whose value does not match the resolved subtype is now silently overridden by the hardcoded constant (e.g. an ASSISTANT payload always yields MsgRole.ASSISTANT). That is the intent, but it is an implicit guarantee.
  2. Msg.validateRoleContent(role, content) now always sees the hardcoded role, so a mismatched stored payload can no longer be detected at all.

A test that deserializes each subtype from JSON with a correct role, a mismatched role, and a missing role, asserting the resulting getRole() and that no exception is thrown, would make the invariant explicit and independent of the base-class annotation.

@oss-maintainer oss-maintainer Sep 11, 2026

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.

[Warning] Removing this parameter does change deserialization behaviour, because Msg declares @JsonTypeInfo(..., visible = true) — the role value from JSON is actively forwarded to the subtype creator rather than consumed only as the type discriminator.

With the parameter gone, role is no longer bindable by this creator, so it is dropped as an unknown property. That is tolerated only because Msg carries @JsonIgnoreProperties(ignoreUnknown = true) and that annotation is @Inherited. So the code compiles and round-trips today, but the cleanup now depends on an inherited annotation on the base class that nothing in this file enforces: if ignoreUnknown were ever narrowed, deserializing these four subtypes would start failing with UnrecognizedPropertyException.

Two consequences worth locking down with a wire-compat test before merge:

  1. A payload whose role is absent or whose value does not match the resolved subtype is now silently overridden by the hardcoded constant (e.g. an ASSISTANT payload always yields MsgRole.ASSISTANT). That is the intent, but it is an implicit guarantee.
  2. Msg.validateRoleContent(role, content) now always sees the hardcoded role, so a mismatched stored payload can no longer be detected at all.

A test that deserializes each subtype from JSON with a correct role, a mismatched role, and a missing role, asserting the resulting getRole() and that no exception is thrown, would make the invariant explicit and independent of the base-class annotation.

@JsonProperty("name") String name,

@oss-maintainer oss-maintainer Sep 11, 2026

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.

Dropping @JsonProperty("role") from these @JsonCreators is only safe because Msg is annotated @JsonIgnoreProperties(ignoreUnknown = true) while @JsonTypeInfo(... include = EXISTING_PROPERTY, property = "role", visible = true) still leaves the role field in the stream. Msg persists agent state that is later recovered from the state store, so a Jackson upgrade or an annotation change that stops the inheritance of ignoreUnknown would turn this into a hard deserialization failure on already-persisted payloads. Please add a round-trip regression test that deserializes a JSON document containing "role":"assistant" into each of the four subtypes (and a Msg-typed read) so this cleanup is actually pinned down — right now the PR has no test.

@JsonProperty("role") MsgRole role,
@JsonProperty("content") List<ContentBlock> content,
@JsonProperty("metadata") Map<String, Object> metadata,
@JsonProperty("timestamp") String timestamp,
Expand Down Expand Up @@ -167,8 +166,7 @@ public Builder generateReason(GenerateReason reason) {

@Override
public AssistantMessage build() {
return new AssistantMessage(
id, name, MsgRole.ASSISTANT, content, metadata, timestamp, usage);
return new AssistantMessage(id, name, content, metadata, timestamp, usage);
}
}
}
3 changes: 1 addition & 2 deletions agentscope-core/src/main/java/io/agentscope/core/message/SystemMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ public SystemMessage(List<ContentBlock> blocks) {
private SystemMessage(
@JsonProperty("id") String id,
@JsonProperty("name") String name,
@JsonProperty("role") MsgRole role,
@JsonProperty("content") List<ContentBlock> content,
@JsonProperty("metadata") Map<String, Object> metadata,
@JsonProperty("timestamp") String timestamp,
Expand Down Expand Up @@ -167,7 +166,7 @@ public Builder generateReason(GenerateReason reason) {

@Override
public SystemMessage build() {
return new SystemMessage(id, name, MsgRole.SYSTEM, content, metadata, timestamp, usage);
return new SystemMessage(id, name, content, metadata, timestamp, usage);
}
}
}
3 changes: 1 addition & 2 deletions agentscope-core/src/main/java/io/agentscope/core/message/ToolResultMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ public ToolResultMessage(List<ToolResultBlock> results) {
private ToolResultMessage(

@oss-maintainer oss-maintainer Sep 11, 2026

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.

Two process points: (1) the PR title says AssistantMessage but SystemMessage, UserMessage and ToolResultMessage change too — please retitle (e.g. refactor(core): drop unused role parameter from message @JsonCreators) since core-interface changes cascade to harness/distribution/extensions; (2) the body's "mvn test passed" line sits above the checklist and the checklist is left at template defaults, so the validation evidence is ambiguous. A mvn -pl agentscope-core test` line would be enough.

@seuCHENGHQ seuCHENGHQ Sep 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Title has been rewritten

@JsonProperty("id") String id,

@oss-maintainer oss-maintainer Sep 11, 2026

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.

[Info] Same visible = true coupling as AssistantMessage.java:73. This subtype is the one where it matters most: ToolResultMessage content blocks are the narrowest (validateRoleContent restricts TOOL), so the role that reaches validation is now fixed rather than supplied by the stored payload — strictly an improvement for state recovery from AgentStateStore, but again only implicitly.

@oss-maintainer oss-maintainer Sep 11, 2026

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.

[Info] Same visible = true coupling as AssistantMessage.java:73. This subtype is the one where it matters most: ToolResultMessage content blocks are the narrowest (validateRoleContent restricts TOOL), so the role that reaches validation is now fixed rather than supplied by the stored payload — strictly an improvement for state recovery from AgentStateStore, but again only implicitly.

@JsonProperty("name") String name,
@JsonProperty("role") MsgRole role,
@JsonProperty("content") List<ContentBlock> content,
@JsonProperty("metadata") Map<String, Object> metadata,
@JsonProperty("timestamp") String timestamp,
Expand Down Expand Up @@ -215,7 +214,7 @@ public Builder generateReason(GenerateReason reason) {
@Override
public ToolResultMessage build() {
return new ToolResultMessage(
id, name, MsgRole.TOOL, List.copyOf(content), metadata, timestamp, usage);
id, name, List.copyOf(content), metadata, timestamp, usage);
}
}
}
3 changes: 1 addition & 2 deletions agentscope-core/src/main/java/io/agentscope/core/message/UserMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ public UserMessage(List<ContentBlock> blocks) {
private UserMessage(
@JsonProperty("id") String id,
@JsonProperty("name") String name,
@JsonProperty("role") MsgRole role,
@JsonProperty("content") List<ContentBlock> content,
@JsonProperty("metadata") Map<String, Object> metadata,
@JsonProperty("timestamp") String timestamp,
Expand Down Expand Up @@ -169,7 +168,7 @@ public Builder generateReason(GenerateReason reason) {

@Override
public UserMessage build() {
return new UserMessage(id, name, MsgRole.USER, content, metadata, timestamp, usage);
return new UserMessage(id, name, content, metadata, timestamp, usage);
}
}
}
Loading