Unify app -> agent naming in handshake metadata (#190)#199
Conversation
Address issue #190 by unifying service naming and correcting misused agent terminology in handshake metadata. Rename app_* metadata keys to agent_* to reflect the intended naming while preserving legacy compatibility during the transition
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #199 +/- ##
=======================================
Coverage 85.61% 85.61%
=======================================
Files 15 15
Lines 6481 6481
=======================================
Hits 5549 5549
Misses 932 932 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pott-cho
left a comment
There was a problem hiding this comment.
Could you make the following changes?
-
Remove the tests added in
lib.rs. I think they are unnecessary for this change. -
Update the app-like test value in
client.rs.
There is still an app-like test value:ConnectionBuilder::new( "test-server", "127.0.0.1:443".parse().unwrap(), "app", ... )
Since this PR renames the client-side metadata to
agent_*, please update this value to something agent-oriented, such as"test-agent", for consistency. -
Revise the CHANGELOG entry.
The current CHANGELOG entry seems a bit misleading. Please change it to something like this:- Renamed handshake metadata fields from `app_name` / `version` to `agent_name` / `agent_version` in `ConnectionBuilder` and `AgentInfo`. -
Fix the CHANGELOG format.
Please change
## Unreleasedto## [Unreleased], and add the[Unreleased]link reference at the bottom of the file. For example:[Unreleased]: https://github.com/petabi/review-protocol/compare/0.19.0...main
Unify handshake metadata by renaming app_name/version to agent_name/agent_version in ConnectionBuilder and AgentInfo. Remove now-redundant agent_info unit tests and unused std::net imports, update certificate-chain tests to use "test-agent", and fix CHANGELOG heading and link references. All tests pass locally (200 tests).
|
I reviewed your comments and made the requested changes — then committed and pushed them. What I changed
Verification
Thanks for the clear review — these updates should address all four items you raised. If you want the CHANGELOG wording adjusted further, I can tweak it. |
| "std", | ||
| ] } | ||
| rustls-pemfile = "2" | ||
| serde_json = "1.0.150" |
There was a problem hiding this comment.
serde_json is no longer used anywhere. Please remove it.
|
I reviewed your comment and found that serde_json was no longer referenced anywhere in the codebase. I removed the unused dependency (serde_json = "1.0.150") from Cargo.toml, committed the change, and pushed it to the branch. This keeps the manifest lean and avoids shipping an unnecessary dependency. I also verified the project builds after the removal. Thanks for pointing that out — it’s been addressed. |
| #[serde(rename = "agent_name", alias = "app_name")] | ||
| pub agent_name: String, | ||
| #[serde(rename = "agent_version", alias = "app_version", alias = "version")] | ||
| pub agent_version: String, |
There was a problem hiding this comment.
Please remove the serde attributes above.
Summary
This PR renames client handshake metadata fields that used
'app_' to 'agent_' to make it explicit that the client-side
component is an agent (not a standalone server). It aligns
ConnectionBuilder and AgentInfo naming, updates tests and the
CHANGELOG, and preserves wire compatibility.
What changed
agent_version (remote_* fields unchanged)
keys (app_name, app_version, version) so existing handshake
payloads continue to be accepted
Compatibility notes
and types unchanged)
legacy key names listed above
Files touched (high level)
References
Closes #190
#190