Phase 0: A2A bugs + configurable agent identity - #47
Conversation
…docs URL Three non-breaking A2A v0.3 fixes plus a configurable agent identity surface for the agent card: - supportedInterfaces[].url now appends '/a2a' (the JSON-RPC mount in register()), matching the spec — clients that POST to the advertised endpoint no longer 404 against the bare base URL. - documentationUrl threaded through a new --docs-url flag, defaulting to the workers repo a2a folder. The previous hardcoded link pointed at iii-hq/iii-connect, which was scrapped. - Identity (name, description, provider org/url) is now driven by AgentIdentity, populated from new --agent-name, --agent-description, --provider-org, --provider-url flags. Defaults preserve the existing hardcoded values byte-for-byte, so no in-flight client breaks. Crate gains a thin lib.rs (re-exports handler + types) so integration tests under a2a/tests/ can drive build_agent_card directly. Three tests cover default identity (a2a suffix + docs URL), trailing-slash normalisation, and full custom identity flow-through.
- Single-source DEFAULT_* consts shared between AgentIdentity::default() and clap default_value attributes
📝 WalkthroughWalkthroughAdds a library target and public entry for the a2a crate, introduces a public Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
a2a/src/handler.rs (1)
208-224:⚠️ Potential issue | 🔴 CriticalRemove leading slashes from both
api_pathvalues; they will cause 404s.The codebase explicitly documents this in
coding/src/templates.rs(lines 725–728): iii-engine prepends/during path matching, so api_paths with leading slashes become double-slash routes (//a2a,//.well-known/agent-card.json) and return 404 at invoke time.Change lines 211 and 220 to:
"api_path": ".well-known/agent-card.json""api_path": "a2a"The agent card advertises
/a2a(line 261); clients will hit that path and get a 404 if these registrations remain unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@a2a/src/handler.rs` around lines 208 - 224, The registered http trigger api_path values include leading slashes causing double-slash routes and 404s; update the two RegisterTriggerInput calls (the iii.register_trigger calls that register "a2a::agent_card" and "a2a::jsonrpc") to remove the leading "/" from the config.api_path entries so they become ".well-known/agent-card.json" and "a2a" respectively, keeping the rest of the RegisterTriggerInput fields unchanged.
🧹 Nitpick comments (3)
a2a/src/handler.rs (2)
260-264: Trailing-slash normalization — small edge case.
base_url.trim_end_matches('/')correctly handleshttp://host:port/and even pathological cases likehttp://host:port///. Two adjacent edge cases worth being aware of (not necessarily fixing now):
- If an operator passes
--base-url http://host:port/a2a(already includes the path), the advertised URL becomeshttp://host:port/a2a/a2a. Unlikely but silent.- Whitespace isn't trimmed —
"http://host:port/ "would advertisehttp://host:port/ /a2a. clap won't trim either.A tiny
let base = base_url.trim().trim_end_matches('/');would cover both cheaply. Pure polish.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@a2a/src/handler.rs` around lines 260 - 264, The advertised A2A URL can end up with extra slashes or embedded whitespace because base_url is only trimmed of trailing slashes; change the construction used when building supported_interfaces/AgentInterface so you first normalize the input (e.g., let base = base_url.trim().trim_end_matches('/')) and then use base in the format! call instead of base_url; update the code that sets supported_interfaces (the AgentInterface url formatting) to reference this normalized base variable to avoid doubled paths and stray spaces.
256-269: Provider is unconditionallySome(...)— consider making it omittable.
provideris always emitted, even if bothprovider_organdprovider_urlare empty strings. The A2A spec treats provider as optional, so a downstream client parsing the card may see{"organization": "", "url": ""}and treat the agent as having a (broken) provider rather than no provider. Two low-effort options:♻️ Either omit when both fields are empty…
- provider: Some(AgentProvider { - organization: identity.provider_org.clone(), - url: identity.provider_url.clone(), - }), + provider: if identity.provider_org.is_empty() && identity.provider_url.is_empty() { + None + } else { + Some(AgentProvider { + organization: identity.provider_org.clone(), + url: identity.provider_url.clone(), + }) + },…or model
AgentIdentity::provider_org/provider_urlasOption<String>end-to-end and let clap'sOption<String>handling do the work. The latter is cleaner but a wider change.Not a blocker for Phase 0 — defaults are non-empty so the wire shape is unchanged today.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@a2a/src/handler.rs` around lines 256 - 269, The AgentCard currently always sets provider to Some(AgentProvider{...}) even when identity.provider_org and identity.provider_url are empty; change the construction to set provider to None when both identity.provider_org and identity.provider_url are empty, otherwise set Some(AgentProvider { organization: identity.provider_org.clone(), url: identity.provider_url.clone() }); update the AgentCard creation site (where AgentCard and AgentProvider are constructed) to use this conditional so the provider field is omitted on the wire when both values are empty.a2a/src/main.rs (1)
95-102: Identity wiring is straightforward — LGTM.CLI args flow into
AgentIdentityand are passed tohandler::register. Defaults preserved via the sharedDEFAULT_*consts, which keeps clap defaults andAgentIdentity::default()in sync.One optional thought: there's no validation that the URL flags (
--provider-url,--docs-url) are well-formed URLs, and empty strings are silently accepted. If an operator typos--agent-name "", the card advertises an empty name. Probably fine for Phase 0, but worth a follow-up if you want defensive trimming/non-empty checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@a2a/src/main.rs` around lines 95 - 102, The CLI currently wires args directly into handler::AgentIdentity and calls handler::register without validating URL or non-empty fields; add simple defensive validation in main before constructing AgentIdentity: trim args.agent_name/agent_description and ensure required fields (e.g., args.agent_name) are non-empty (return an error or exit with a clear message), and validate args.provider_url and args.docs_url by attempting to parse them as URLs (e.g., using url::Url::parse) and reject/notify on invalid values; alternatively make those fields Option<String> in AgentIdentity and only set them when non-empty/valid so handler::register receives sanitized inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@a2a/src/handler.rs`:
- Around line 208-224: The registered http trigger api_path values include
leading slashes causing double-slash routes and 404s; update the two
RegisterTriggerInput calls (the iii.register_trigger calls that register
"a2a::agent_card" and "a2a::jsonrpc") to remove the leading "/" from the
config.api_path entries so they become ".well-known/agent-card.json" and "a2a"
respectively, keeping the rest of the RegisterTriggerInput fields unchanged.
---
Nitpick comments:
In `@a2a/src/handler.rs`:
- Around line 260-264: The advertised A2A URL can end up with extra slashes or
embedded whitespace because base_url is only trimmed of trailing slashes; change
the construction used when building supported_interfaces/AgentInterface so you
first normalize the input (e.g., let base =
base_url.trim().trim_end_matches('/')) and then use base in the format! call
instead of base_url; update the code that sets supported_interfaces (the
AgentInterface url formatting) to reference this normalized base variable to
avoid doubled paths and stray spaces.
- Around line 256-269: The AgentCard currently always sets provider to
Some(AgentProvider{...}) even when identity.provider_org and
identity.provider_url are empty; change the construction to set provider to None
when both identity.provider_org and identity.provider_url are empty, otherwise
set Some(AgentProvider { organization: identity.provider_org.clone(), url:
identity.provider_url.clone() }); update the AgentCard creation site (where
AgentCard and AgentProvider are constructed) to use this conditional so the
provider field is omitted on the wire when both values are empty.
In `@a2a/src/main.rs`:
- Around line 95-102: The CLI currently wires args directly into
handler::AgentIdentity and calls handler::register without validating URL or
non-empty fields; add simple defensive validation in main before constructing
AgentIdentity: trim args.agent_name/agent_description and ensure required fields
(e.g., args.agent_name) are non-empty (return an error or exit with a clear
message), and validate args.provider_url and args.docs_url by attempting to
parse them as URLs (e.g., using url::Url::parse) and reject/notify on invalid
values; alternatively make those fields Option<String> in AgentIdentity and only
set them when non-empty/valid so handler::register receives sanitized inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e40cb982-8e66-4101-9a9f-3cd7b4601d17
⛔ Files ignored due to path filters (1)
a2a/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
a2a/Cargo.tomla2a/src/handler.rsa2a/src/lib.rsa2a/src/main.rsa2a/tests/agent_card.rs
…tional provider/docs URL
- api_path: '.well-known/agent-card.json' / 'a2a' (no leading slash; engine
prepends '/' so leading slash produces double-slash 404)
- base_url normalised via .trim().trim_end_matches('/')
- AgentCard provider/documentation_url omitted when fields empty (spec-optional)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
a2a/tests/agent_card.rs (1)
22-50: Optional: tighten default-identity coverage.The default-identity test asserts
name, provider, and docs URL but skipscard.description. SinceDEFAULT_AGENT_DESCRIPTIONis part of the same single-source-of-truth inhandler.rs(line 70-71) and is what users will see in clients, asserting it here closes the loop on accidental drift.Also, the PR objective lists whitespace trimming on
base_urlas part of the normalization, but only trailing-slash is exercised intrailing_slash_in_base_url_is_normalised(line 53). A one-line case (e.g.," http://localhost:3111 ") would lock that behavior in too.♻️ Proposed additions
assert_eq!(card.name, "iii-engine"); + assert_eq!( + card.description, + "iii-engine agent — invoke any registered function via A2A" + ); let provider = card .provider .expect("default identity always has a provider");#[tokio::test] async fn trailing_slash_in_base_url_is_normalised() { let iii = unreachable_iii(); let cfg = ExposureConfig::new(false, None); let identity = AgentIdentity::default(); let card = build_agent_card(&iii, &cfg, "http://localhost:3111/", &identity).await; assert_eq!( card.supported_interfaces[0].url, "http://localhost:3111/a2a", "trailing slash on base_url must not produce a doubled `//a2a`" ); + + let card_ws = + build_agent_card(&iii, &cfg, " http://localhost:3111/ ", &identity).await; + assert_eq!( + card_ws.supported_interfaces[0].url, "http://localhost:3111/a2a", + "surrounding whitespace on base_url must be trimmed before /a2a is appended" + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@a2a/tests/agent_card.rs` around lines 22 - 50, Add assertions to the test default_identity_advertises_a2a_suffix_and_docs_url to verify card.description equals the canonical DEFAULT_AGENT_DESCRIPTION from handler.rs (so changes to AgentIdentity::default()/handler.rs are caught), and add one small sub-case calling build_agent_card with a base_url containing surrounding whitespace (e.g., " http://localhost:3111 ") to assert supported_interfaces[0].url still normalizes to "http://localhost:3111/a2a"; update references to AgentIdentity::default(), build_agent_card, and DEFAULT_AGENT_DESCRIPTION to locate the checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@a2a/src/handler.rs`:
- Around line 252-259: The current provider construction uses a conjunction so
provider becomes None only when both identity.provider_org and
identity.provider_url are empty; change the conditional to use logical OR so
provider is set to None if either identity.provider_org.is_empty() or
identity.provider_url.is_empty(), and otherwise construct Some(AgentProvider {
organization: ..., url: ... }); update the conditional around the provider
variable generation in handler.rs to mirror the documentation_url pattern and
ensure AgentProvider never contains a half-empty field.
---
Nitpick comments:
In `@a2a/tests/agent_card.rs`:
- Around line 22-50: Add assertions to the test
default_identity_advertises_a2a_suffix_and_docs_url to verify card.description
equals the canonical DEFAULT_AGENT_DESCRIPTION from handler.rs (so changes to
AgentIdentity::default()/handler.rs are caught), and add one small sub-case
calling build_agent_card with a base_url containing surrounding whitespace
(e.g., " http://localhost:3111 ") to assert supported_interfaces[0].url still
normalizes to "http://localhost:3111/a2a"; update references to
AgentIdentity::default(), build_agent_card, and DEFAULT_AGENT_DESCRIPTION to
locate the checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b54299bf-97ad-4176-ac5a-82ed6fc98371
⛔ Files ignored due to path filters (1)
a2a/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
a2a/Cargo.tomla2a/src/handler.rsa2a/tests/agent_card.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- a2a/Cargo.toml
…ompliance) Both AgentProvider.organization and AgentProvider.url are required by the A2A v0.3 spec. Half-populated provider violates the spec; switch && to || so we omit the field entirely if either is missing.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
a2a/src/handler.rs (1)
127-131: Drop the unnecessaryidentity.clone()on line 131.
identityis an ownedAgentIdentityand isn't referenced again after this binding, so the clone is wasted work — you can move it straight intocard_identity. The closure still needs its ownident = card_identity.clone()since it's aFn-style handler invoked repeatedly.♻️ Proposed tweak
-pub fn register(iii: &III, exposure: ExposureConfig, base_url: String, identity: AgentIdentity) { +pub fn register(iii: &III, exposure: ExposureConfig, base_url: String, identity: AgentIdentity) { let iii_card = iii.clone(); let card_cfg = exposure.clone(); let card_base_url = base_url.clone(); - let card_identity = identity.clone(); + let card_identity = identity;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@a2a/src/handler.rs` around lines 127 - 131, In register, you're cloning `identity` unnecessarily into `card_identity`: since `identity` is owned and not used afterward, move it directly into `card_identity` (i.e., take ownership instead of calling `identity.clone()`), and keep the existing `ident = card_identity.clone()` inside the Fn-style handler closure so the closure can clone per invocation; update the binding of `card_identity` in the `register` function to use ownership transfer rather than cloning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@a2a/src/handler.rs`:
- Around line 127-131: In register, you're cloning `identity` unnecessarily into
`card_identity`: since `identity` is owned and not used afterward, move it
directly into `card_identity` (i.e., take ownership instead of calling
`identity.clone()`), and keep the existing `ident = card_identity.clone()`
inside the Fn-style handler closure so the closure can clone per invocation;
update the binding of `card_identity` in the `register` function to use
ownership transfer rather than cloning.
1 participant
Tracks #45 — Phase 0 of MCP+A2A overhaul.
Summary
Three non-breaking A2A fixes plus configurable agent identity:
serviceEndpoint/a2asuffix. Agent card was advertising barebase_url; spec-compliant clients hit root and 404'd. Now advertises<base_url>/a2awith trailing-slash normalisation.documentation_urlconfigurable. Old hardcoded value pointed at the scrappediii-connectrepo. Now read from--docs-urlflag, defaulthttps://github.com/iii-hq/workers/tree/main/a2a.--agent-name,--agent-description,--provider-org,--provider-url,--docs-url. Defaults preserve current behavior except for the dead docs URL.DEFAULT_*consts shared betweenAgentIdentity::default()and clap defaults.[lib]target added so integration tests can exercisebuild_agent_carddirectly.Test plan
cargo check -p iii-a2acleancargo test -p iii-a2agreen (3/3)cargo run --release -p iii-a2a -- --agent-name "acme-ops"→curl /.well-known/agent-card.json | jq '.supportedInterfaces[0].url'ends with/a2aSequencing
Land first — every other phase branch carries the dead docs URL and missing
/a2asuffix and needs to inherit this fix.Summary by CodeRabbit
New Features
Tests