fix(core): derive Version.VERSION from Maven project version instead of hardcoding - #3087
fix(core): derive Version.VERSION from Maven project version instead of hardcoding#3087amyaxy wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
3c64568 to
36e9331
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Deriving Version.VERSION from the Maven project version removes a hardcoded string that had already gone stale — good direction, and the unfiltered-placeholder guard plus the extra unit tests show real care. The blocker is that the two overlapping <resource> blocks may leave the unfiltered file in the jar, and the "unknown" fallback would hide that.
Findings
- [Critical]
pom.xml:211— catch-all unfiltered resource + filtered resource writing the same target path; without<excludes>(oroverwrite) the filtered copy can be skipped, shipping the literal${project.version}. - [Warning]
Version.java:53—"unknown"in the User-Agent / MCP client info / telemetry version is a regression vs. the previous constant for any non-Maven-filtered runtime. - [Info]
version.properties:1— no trailing newline. - [Info]
VersionTest.java:48— theAssumptionsskip can swallow exactly the packaging bug above.
Suggestions
<resource>
<directory>src/main/resources</directory>
<filtering>false</filtering>
<excludes>
<exclude>META-INF/agentscope/version.properties</exclude>
</excludes>
</resource>
<resource>
<directory>src/main/resources</directory>
<filtering>true</filtering>
<includes>
<include>META-INF/agentscope/version.properties</include>
</includes>
</resource>Then verify the artifact directly in CI, e.g. unzip -p agentscope-core/target/*.jar META-INF/agentscope/version.properties | grep -q '${' && exit 1.
Automated review by github-manager-bot
| <build> | ||
| <resources> | ||
| <!-- Non-filtered default: avoids mangling future ${...} in other resources. --> | ||
| <resource> |
There was a problem hiding this comment.
Relying on declaration order to make the filtered copy win is fragile here: both resource blocks read the same src/main/resources tree and the same target path, and maven-resources-plugin skips a copy when the destination is not older than the source (default overwrite=false). Since the unfiltered copy runs first and rewrites target/classes/META-INF/agentscope/version.properties with a newer timestamp than the source, the filtered copy can be skipped and the packaged jar ends up containing the literal ${project.version}. Version.resolveVersionFrom then masks that by returning "unknown", so the failure is silent rather than loud. Safer: drop the catch-all unfiltered block's duplicate and exclude the version file from it, e.g. keep one <resource> for src/main/resources with <excludes><exclude>META-INF/agentscope/version.properties</exclude></excludes> plus the filtered resource — then add an assertion (or a maven-enforcer/groovy check) that the packaged resource no longer contains ${.
| @@ -0,0 +1 @@ | |||
| version=${project.version} No newline at end of file | |||
There was a problem hiding this comment.
Missing trailing newline (note the \ No newline at end of file marker). Harmless for Properties.load, but spotless/POSIX hygiene tools flag it.
| } catch (IOException e) { | ||
| // Fall through to the fallback below. | ||
| } | ||
| return "unknown"; |
There was a problem hiding this comment.
getUserAgent() is used as a real User-Agent for model-provider traffic (and Version.VERSION feeds McpClientBuilder client info and TelemetryTracer instrumentation version). Falling back to "unknown" means every run that resolves outside a Maven-filtered classpath — IDE run configs, java -cp target/classes, shaded/relocated jars that drop META-INF/agentscope, custom packaging — now reports agentscope-java/unknown, which is worse telemetry than the previous hardcoded constant. Consider keeping a build-time fallback that is still meaningful (e.g. a generateGitPropertiesFile/impl-version from the manifest's Implementation-Version, then the old hardcoded value) and logging once at debug/warn when the resource is unresolved.
| // value is an unfiltered ${project.version} literal and there is no real version to | ||
| // cross-check, so skip the strict assertions. CI runs Maven, so filtering always applies. | ||
| boolean filtered = !expectedVersion.contains("${"); | ||
| Assumptions.assumeTrue( |
There was a problem hiding this comment.
Assumptions.assumeTrue(filtered, ...) makes the strict cross-check self-skipping, so the scenario described above (Maven did run but the filtered resource was not applied) is reported as a pass/skip instead of a failure. If the resource is present on the test classpath at all, an unfiltered ${project.version} is arguably a genuine failure rather than an assumption violation — maybe gate the skip on the resource being absent, not on the value being unfiltered.
…gentscope-ai#3087) - core pom: exclude version.properties from the unfiltered catch-all so the filtered block is the sole writer of the target file; removes the fragile declaration-order race that could package the literal placeholder - core pom: add maven-antrun build check (process-classes) that fails loudly when the filtered resource still contains the unmapped placeholder - parent pom: enable addDefaultImplementationEntries so module jars record Implementation-Version (fallback for shaded/custom packaging) - Version: three-level fallback chain (filtered resource -> manifest Implementation-Version -> 'unknown') with one-time warning log; dropped the stale hardcoded constant as fallback to avoid reintroducing drift - VersionTest: skip only when the resource is absent; an unfiltered value is now a hard failure; add manifest/resource/IO branch tests (cover codecov) - version.properties files: add trailing newline
4aeb839 to
65bd6b6
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review after c4eb8bef: all four findings from the previous round are properly resolved — the unfiltered resource block now excludes META-INF/agentscope/version.properties, so the filtered block is the sole writer and the declaration-order race is gone; the "unknown" fallback is now a three-level chain with a jar-manifest Implementation-Version rung plus a one-time warning; the antrun check fails the build loudly instead of silently shipping the placeholder; and the test no longer self-skips the exact packaging bug it was written to catch (it only aborts when the resource is genuinely absent). New tests cover the manifest / resource / IO branches.
CI at review time: Check License, Check Module Sync, build (ubuntu-latest), codecov/patch and license/cla are green, build (windows-latest) was still queued.
The three remaining notes are Info-level only (match project.version instead of a bare $, tolerate a missing file in the antrun condition, and the parent-pom manifest scope) — none of them block merging.
LGTM, and thanks for the thorough follow-up.
Automated review by github-manager-bot
| <!-- A single '$' is enough to detect the unmapped | ||
| ${project.version} placeholder: the filtered file is | ||
| just 'version=2.0.3-SNAPSHOT' and contains none. --> | ||
| <resourcecontains resource="${project.build.outputDirectory}/META-INF/agentscope/version.properties" substring="$" /> |
There was a problem hiding this comment.
Good addition - this is the loud failure the previous round asked for. One nit: substring="$" is a blunt instrument, because any future $ in the file (a comment line, a build-metadata entry) would break the build for an unrelated reason. The thing you actually want to reject is the literal ${project.version} placeholder, and since Maven interpolates ${...} inside the POM, matching on project.version gets you that precision without any escaping trick:
<resourcecontains resource="${project.build.outputDirectory}/META-INF/agentscope/version.properties" substring="project.version" />A correctly filtered file (version=2.0.3-SNAPSHOT) can never contain that token.
| <executions> | ||
| <execution> | ||
| <id>verify-version-resource-filtered</id> | ||
| <phase>process-classes</phase> |
There was a problem hiding this comment.
process-classes runs on every mvn compile, so this execution also fires in builds that never produced target/classes/META-INF/agentscope/version.properties (resources phase skipped, stale/partial target/). Ant's resourcecontains generally reports a missing resource as its own error, which would replace the curated fail message with something less actionable. If you want the diagnostic to survive those cases, gate it first, e.g. fail when <not><available file="${project.build.outputDirectory}/META-INF/agentscope/version.properties"/></not>, or combine the two conditions with <and>. Not blocking - CI and normal builds are covered.
| Version can fall back to Implementation-Version when the filtered | ||
| version.properties resource is absent from the classpath (e.g. | ||
| shaded/relocated jars or custom packaging). --> | ||
| <addDefaultImplementationEntries>true</addDefaultImplementationEntries> |
There was a problem hiding this comment.
Heads-up on blast radius rather than a defect: this sits in the parent's <build><plugins>, so Implementation-Title / Implementation-Version / Implementation-Vendor now land in every module jar's manifest, not just agentscope-core. That is harmless (and useful for support tooling), but if you want to keep the change scoped to the module that reads it, move the plugin block into agentscope-core/pom.xml. Worth noting the shaded agentscope-all artifact already carries core's filtered META-INF/agentscope/version.properties, so level 1 covers it and the manifest entry is mostly a safety net for custom packaging / Spring Boot fat jars where Package#getImplementationVersion() can come back null.
…of hardcoding Fixes agentscope-ai#3086 Resolve the version at build time instead of keeping a hardcoded constant that drifts from the pom: - agentscope-core pom: filter only META-INF/agentscope/version.properties (${project.version}); the unfiltered catch-all resource block excludes it so the filtered block is the sole writer of the target file. This removes the fragile declaration-order race where the unfiltered copy (running first) rewrites the target with a newer timestamp and makes maven-resources-plugin skip the filtered copy, silently packaging the literal placeholder. - agentscope-core pom: add a maven-antrun build check (process-classes phase) that fails loudly when the filtered resource still contains an unmapped placeholder. - parent pom: enable maven-jar-plugin addDefaultImplementationEntries so module jars record Implementation-Version, used as the fallback when the classpath resource is missing (shaded/relocated or custom packaging). - Version: three-level fallback chain - filtered classpath resource, jar manifest Implementation-Version, then "unknown" with a one-time warning log; blank and unfiltered ${...} values are rejected. - VersionTest: skip only when the version resource is absent from the classpath (IDE/non-Maven run); an unfiltered value present on the classpath is now a hard failure; adds manifest/resource/IO branch tests. - version.properties files: add trailing newline. Verified locally: mvn process-classes and VersionTest 21/21 green; the packaged jar manifest contains Implementation-Version: 2.0.3-SNAPSHOT.
65bd6b6 to
00195b7
Compare
2 participants
Fixes #3086
AgentScope-Java Version
2.0.3
Description
Background
Version.VERSIONwas a hardcoded constant (1.0.13-SNAPSHOT) that haddrifted from the actual Maven project version (
2.0.3-SNAPSHOTinpom.xmlvia the CI-friendly
<revision>property), so callers of theVersionAPIcould receive a stale version string at runtime.
Purpose
Make Maven
<revision>the single source of truth for the runtime version,eliminating manual synchronization between
pom.xmlandVersion.java.Changes
agentscope-core/src/main/resources/META-INF/agentscope/version.properties(new): filtered with
${project.version}during the buildagentscope-core/src/main/java/io/agentscope/core/Version.java: loadVERSIONfrom the classpath resource at runtime; fall back to"unknown"if an unfiltered build leaks the literal
${placeholderagentscope-core/pom.xml: enable resource filtering for the versionproperties file only; keep all other resources unfiltered
agentscope-core/src/test/resources/agentscope-test-version.properties(new): test-only filtered resource for cross-verification
agentscope-core/src/test/java/io/agentscope/core/VersionTest.java: assertVersion.VERSIONequals the Maven-resolved version and matchessemantic-versioning; gracefully skip under non-Maven (IDEA) builds
How to test
Checklist
mvn spotless:applymvn test) — pending CI confirmation