feat: implement draft-hardt-oauth-aauth-protocol across the mesh#472
Merged
Conversation
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryHigh Risk Overview Gateway: AAuth now runs in Supporting: Reviewed by Cursor Bugbot for commit 7d78847. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds shared AAuth wire types, verifier and SDK libraries, Access Server and Person Server flows, gateway runtime wiring, and JWKS publishing support. ChangesAAuth platform stack
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 7d78847 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
rsworkspace/crates/a2a-nats/src/gateway_ingress.rs (1)
237-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider exporting
-32_118as a named constant.The literal is duplicated in
a2a-gateway's dispatch audit code (AuditOutcome::Err { code: -32_118, ... }). Exporting it here (mirroringa2a_gateway::aauth::AAUTH_REQUIRED_CODEas the doc states) and having dispatch.rs reference it would eliminate the drift risk between the two crates.♻️ Proposed constant export
+/// Mirrors `a2a_gateway::aauth::AAUTH_REQUIRED_CODE`. +pub const AAUTH_DENIED_CODE: i32 = -32_118; + pub fn ingress_gateway_aauth_denied_response_bytes( request_headers: &HeaderMap, request_payload_hint: &[u8], message: impl Into<String>, ) -> Result<bytes::Bytes, WireError> { - Ok(ingress_error_wire(request_headers, request_payload_hint, -32_118, message, None)?.body) + Ok(ingress_error_wire(request_headers, request_payload_hint, AAUTH_DENIED_CODE, message, None)?.body) }#!/bin/bash # Confirm the AAUTH_REQUIRED_CODE constant this comment claims to mirror. rg -n 'AAUTH_REQUIRED_CODE' rsworkspace/crates/a2a-gateway🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/a2a-nats/src/gateway_ingress.rs` around lines 237 - 245, The AAUTH denial status code is hardcoded in ingress_gateway_aauth_denied_response_bytes and duplicated elsewhere, so it should be exported as a named constant instead of using the literal. Introduce a shared constant near ingress_gateway_aauth_denied_response_bytes in a2a-nats (mirroring a2a_gateway::aauth::AAUTH_REQUIRED_CODE) and update dispatch.rs in a2a-gateway to reference that constant so both crates stay aligned.rsworkspace/crates/a2a-gateway/src/runtime/dispatch.rs (1)
296-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
-32_118duplicates the code baked intoingress_gateway_aauth_denied_response_bytes.If the canonical
AAUTH_REQUIRED_CODEreferenced ina2a-nats's doc comment ever changes, this audit literal can silently drift out of sync. See companion comment ingateway_ingress.rssuggesting an exported constant to reference from here instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/a2a-gateway/src/runtime/dispatch.rs` around lines 296 - 302, The audit outcome in dispatch.rs hardcodes the AAUTH rejection code, which can drift from the canonical value used by ingress response handling. Update the code path around the AuditOutcome::Err in dispatch to reference a shared exported AAUTH_REQUIRED_CODE constant from the gateway ingress/aauth module instead of repeating the numeric literal, so dispatch.rs and ingress_gateway_aauth_denied_response_bytes stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rsworkspace/crates/a2a-gateway/src/runtime/aauth_env.rs`:
- Around line 187-205: Both optional_u64 and optional_i64 are discarding the
ParseIntError from .parse(), so the resulting AAuthEnvError loses useful
diagnostics. Update the AAuthEnvError variant used here to carry the parse error
as a #[source] field, and change both helpers to pass through the original error
instead of mapping it to an underscore. Keep the raw input in the error too, but
preserve the typed source from the parse call for better overflow/invalid-digit
reporting.
- Around line 152-157: The required_var helper in aauth_env.rs accepts non-empty
env values but returns them untrimmed, so trailing newlines from secret-mounted
files can leak into callers like jwks_path, challenge_key_path, resource_iss,
person_server_aud, and challenge_kid. Update required_var to normalize the value
before returning it, keeping the existing empty-check but returning the trimmed
string instead of the original. Make sure AAuthEnvError::MissingRequired
behavior stays the same for blank values.
- Around line 97-111: The `NonNegativeSecs::new` error path in `aauth_env.rs` is
discarding the typed validation error and re-reading the env var via
`env.var(...)`. Update the parsing flow around `optional_i64`,
`NonNegativeSecs::new`, and the `AAuthEnvError::InvalidNonNegativeSecs` mapping
so the raw value is captured once and reused. Preserve the original
`NonNegativeSecs` error as a source field or variant instead of converting it
away, and avoid the extra env lookup in both `challenge_ttl_secs` and
`max_skew_secs`.
---
Nitpick comments:
In `@rsworkspace/crates/a2a-gateway/src/runtime/dispatch.rs`:
- Around line 296-302: The audit outcome in dispatch.rs hardcodes the AAUTH
rejection code, which can drift from the canonical value used by ingress
response handling. Update the code path around the AuditOutcome::Err in dispatch
to reference a shared exported AAUTH_REQUIRED_CODE constant from the gateway
ingress/aauth module instead of repeating the numeric literal, so dispatch.rs
and ingress_gateway_aauth_denied_response_bytes stay in sync.
In `@rsworkspace/crates/a2a-nats/src/gateway_ingress.rs`:
- Around line 237-245: The AAUTH denial status code is hardcoded in
ingress_gateway_aauth_denied_response_bytes and duplicated elsewhere, so it
should be exported as a named constant instead of using the literal. Introduce a
shared constant near ingress_gateway_aauth_denied_response_bytes in a2a-nats
(mirroring a2a_gateway::aauth::AAUTH_REQUIRED_CODE) and update dispatch.rs in
a2a-gateway to reference that constant so both crates stay aligned.
🪄 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: d0c69d62-9029-4efd-9cf8-e7c6981d5e1c
📒 Files selected for processing (8)
rsworkspace/crates/a2a-gateway/src/runtime.rsrsworkspace/crates/a2a-gateway/src/runtime/aauth_env.rsrsworkspace/crates/a2a-gateway/src/runtime/aauth_env/tests.rsrsworkspace/crates/a2a-gateway/src/runtime/dispatch.rsrsworkspace/crates/a2a-nats/src/gateway_ingress.rsrsworkspace/crates/a2a-nats/src/gateway_ingress/tests.rsrsworkspace/crates/a2a-nats/src/lib.rsrsworkspace/crates/trogon-identity-types/src/aauth/headers.rs
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…g draft verification rules Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…wn jwks publishing Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…ty, enforce scopes and missions Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…ect negative jwks ttl Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (4)
rsworkspace/crates/trogon-aauth-as/src/subagent.rs-36-45 (1)
36-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
BadShapecheck doesn't actually enforce three segments.
splitn(3, '.')combined with only twoparts.next()calls accepts a two-segment string like"header.payload"(no signature segment) without error, even though the doc/error text says "expected three dot-separated segments." In practice this is low-risk since callers only pass already-signature-verified JWTs (which structurally must have 3 segments), but the shape check itself doesn't match its own contract.🐛 Proposed fix to actually check for three segments
let mut parts = verified_jwt.splitn(3, '.'); let _header = parts.next().ok_or(ParentAgentError::BadShape)?; let payload_b64 = parts.next().ok_or(ParentAgentError::BadShape)?; + if parts.next().is_none() { + return Err(ParentAgentError::BadShape); + } let payload = URL_SAFE_NO_PAD🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-as/src/subagent.rs` around lines 36 - 45, The shape validation in parent_agent_of only consumes two split segments, so it can accept a token with no signature segment even though BadShape implies three dot-separated parts. Update the parsing in parent_agent_of to explicitly verify that the JWT has exactly three segments before decoding the payload, and keep the existing ParentAgentError::BadShape path for any malformed input.rsworkspace/crates/a2a-gateway/src/runtime/aauth_env.rs-234-239 (1)
234-239: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject negative JWKS TTLs here. A negative
A2A_GATEWAY_AAUTH_JWKS_TTL_SECSmakes every cached JWK entry expire immediately, so discovery stops caching and re-fetches on every verification. Clamp or validate this at env parsing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/a2a-gateway/src/runtime/aauth_env.rs` around lines 234 - 239, The JWKS TTL parsing in well_known_resolver_from_env currently accepts negative values from optional_i64, which causes CachedJwksResolver to effectively disable caching; validate or clamp ENV_AAUTH_JWKS_TTL_SECS to a non-negative value before passing it into CachedJwksResolver::new(...).with_ttl_secs(...), and return an AAuthEnvError if the env value is negative so the bad configuration is rejected early.rsworkspace/crates/trogon-aauth-sdk/src/subagent.rs-54-56 (1)
54-56: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDecode errors are silently discarded, conflating malformed tokens with "not a sub-agent token".
decode_payloaddrops the underlyingbase64/serde_jsonerror viamap_err(|_| ...), andparent_agent_offurther swallows any decode failure intoNone. As a result,build_subagent_token_requestreportsNotASubAgentTokenfor a genuinely malformedsubagent_agent_jwt, hiding the real cause. As per coding guidelines,**/*.rs: "Never discard error context by converting a typed error into a string; wrap the source error as a field or variant instead."🩹 Proposed fix to preserve error context
pub enum SubAgentError { ... /// The agent token's payload segment could not be decoded as JSON. - #[error("agent token payload could not be decoded")] - MalformedAgentToken, + #[error("agent token payload could not be decoded: {0}")] + MalformedAgentToken(String), } fn decode_payload(jwt: &str) -> Result<ParentAgentOnly, SubAgentError> { let mut parts = jwt.splitn(3, '.'); let _ = parts.next(); - let payload_b64 = parts.next().ok_or(SubAgentError::MalformedAgentToken)?; + let payload_b64 = parts + .next() + .ok_or_else(|| SubAgentError::MalformedAgentToken("missing payload segment".into()))?; let payload = URL_SAFE_NO_PAD .decode(payload_b64.as_bytes()) - .map_err(|_| SubAgentError::MalformedAgentToken)?; - serde_json::from_slice(&payload).map_err(|_| SubAgentError::MalformedAgentToken) + .map_err(|e| SubAgentError::MalformedAgentToken(e.to_string()))?; + serde_json::from_slice(&payload).map_err(|e| SubAgentError::MalformedAgentToken(e.to_string())) }Then have
build_subagent_token_requestpropagatedecode_payload'sResultdirectly forsubagent_agent_jwtinstead of routing throughparent_agent_of'sOption, so malformed input surfaces asMalformedAgentTokenrather thanNotASubAgentToken.Also applies to: 64-72, 106-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-sdk/src/subagent.rs` around lines 54 - 56, Decode failures in parent_agent_of are being collapsed into None, which hides malformed token errors and turns them into NotASubAgentToken. Update parent_agent_of and the decode_payload flow in subagent.rs to preserve and propagate the underlying decode error instead of discarding it, and make build_subagent_token_request return the decode failure directly for subagent_agent_jwt so malformed input surfaces as MalformedAgentToken. Keep the error context attached in the typed error path rather than converting it to a string or Option.Source: Coding guidelines
rsworkspace/crates/trogon-aauth-verify/src/mission.rs-13-16 (1)
13-16: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the doc link to
MissionRef
The module docs link to[MissionClaim], but this crate usesMissionRefhere, so rustdoc will emit a broken intra-doc link warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-verify/src/mission.rs` around lines 13 - 16, The module docs contain a broken intra-doc link because they refer to MissionClaim, but this crate actually uses MissionRef in this context. Update the doc comment near extract_mission_claim and AuthClaims to point the link at MissionRef so rustdoc resolves it correctly and the documentation stays consistent with the types used here.
🧹 Nitpick comments (20)
rsworkspace/crates/trogon-aauth-sdk/Cargo.toml (1)
26-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
trogon-aauth-verifydev-dependency.It's already listed under
[dependencies](line 26) with identicalworkspace = true, making the[dev-dependencies]re-declaration (line 33) redundant since regular dependencies are already available to tests.♻️ Proposed fix
[dev-dependencies] jsonwebtoken = { version = "=10.4.0", features = ["rust_crypto"] } tokio = { workspace = true, features = ["rt", "rt-multi-thread", "macros", "time"] } -trogon-aauth-verify = { workspace = true } wiremock = { workspace = true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-sdk/Cargo.toml` around lines 26 - 34, Remove the redundant test-only re-declaration of trogon-aauth-verify in Cargo.toml for trogon-aauth-sdk, since it is already available through the existing [dependencies] entry with workspace = true. Keep the dependency only once in the manifest and leave the other dev-dependencies unchanged; use the trogon-aauth-verify and [dev-dependencies] entries to locate the duplicate.rsworkspace/crates/trogon-identity-types/src/aauth/mission.rs (1)
106-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuses hand-rolled parsing instead of shared
split_headerhelper.
Requirement::parseinmod.rsusessplit_headerand tolerates malformed segments gracefully;MissionHeader::parsere-implements similarkey="value"; key="value"parsing but bails entirely (?) on any single malformed segment. Consider factoring both onto a shared helper for consistent behavior and to avoid parsing-logic duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-identity-types/src/aauth/mission.rs` around lines 106 - 129, MissionHeader::parse is duplicating header parsing logic instead of using the shared split_header helper used by Requirement::parse, and it currently aborts on any malformed segment rather than handling invalid parts consistently. Update MissionHeader::parse in mission.rs to reuse split_header (or factor both parsers onto a common helper) so parsing behavior matches the existing tolerant approach, while still extracting approver and s256 from the parsed segments.rsworkspace/crates/trogon-jwks-publisher/src/publisher.rs (2)
116-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDwk filenames stored as raw
Stringrather than a validated domain type.
JwksPublisherConfigBuilder::entriesandJwksPublisherConfig::entrieskey on rawString, with correctness enforced only insidewith_jwk_setat call time. Per this repo's coding guideline to prefer domain-specific value objects over primitives so invalid instances are unrepresentable, consider aDwkFilename(or similar) enum/newtype with a fallible constructor (TryFrom<&str>), used as theHashMapkey and as thePathextractor type inserve_dwk. That would let the type system (rather than a runtime check duplicated per call site) guarantee only the four registered filenames are ever handled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-jwks-publisher/src/publisher.rs` around lines 116 - 168, The builder and config currently store dwk names as raw String keys, so validity is only checked at insertion time in JwksPublisherConfigBuilder::with_jwk_set. Introduce a domain-specific DwkFilename newtype/enum with a fallible constructor and use it in JwksPublisherConfigBuilder::entries, JwksPublisherConfig::entries, and the serve_dwk path extractor so only valid registered filenames can exist by type. Keep the existing uniqueness and known-dwk checks at the conversion boundary, and update all lookups/inserts to use the validated type instead of String.Source: Coding guidelines
80-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent fallback to empty JWK coordinates on decode failure, unlike the sibling SDK helper.
point.x().map(base64_url).unwrap_or_default()(and the same fory()on line 91) silently produces an empty-string coordinate ifx()/y()ever returnNone, rather than surfacing an error. The comment above justifies this via the invariant that an uncompressed P-256 encoded point always carries both coordinates for a non-identity key — which is a defensible invariant given a valid signing key — but it means this function can never signal a problem here even though the equivalenttrogon-aauth-sdk::signer::public_jwk(which shares near-identical logic) returns a typed error (AgentSignerError::InvalidPublicKey) in the same situation. If the two implementations were unified (see also the comment onsigner.rs), this divergence would be eliminated and any future violation of the invariant would surface as an error instead of a malformed/empty-coordinate JWK being published at.well-known.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-jwks-publisher/src/publisher.rs` around lines 80 - 106, The JWK builder in jwk_from_ec_pkcs8_pem currently falls back to empty x/y coordinates when the encoded point is missing them, which hides a malformed key condition. Update this path to validate both point.x() and point.y() explicitly and return a PublisherError (or the shared public-key error used by the sibling helper) instead of unwrap_or_default, so invalid EC keys fail fast rather than producing an empty-coordinate JWK. If possible, align this logic with the equivalent public_jwk implementation to keep both code paths consistent.rsworkspace/crates/trogon-aauth-sdk/src/signer.rs (1)
171-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDuplicate EC→JWK coordinate-extraction logic vs.
trogon-jwks-publisher.
public_jwkhere duplicates the P-256 point→JWK coordinate logic introgon-jwks-publisher/src/publisher.rs::jwk_from_ec_pkcs8_pem(that file's own comment acknowledges "mirroringtrogon-aauth-sdk'spublic_jwkhelper"). The two implementations have already diverged in error handling: this one returnsErr(AgentSignerError::InvalidPublicKey)ifx()/y()returnNone(lines 174-175), while the publisher's version silently falls back to an empty string viaunwrap_or_default(). See the companion comment onpublisher.rs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-sdk/src/signer.rs` around lines 171 - 182, The P-256 point-to-JWK coordinate extraction is duplicated between public_jwk and jwk_from_ec_pkcs8_pem, and the two copies have diverged in how missing coordinates are handled. Update the implementation so both paths share the same logic or at least the same error behavior, and make the publisher-side helper match public_jwk by returning an explicit InvalidPublicKey-style error instead of silently using empty coordinates.rsworkspace/crates/trogon-aauth-sdk/src/delegation.rs (1)
27-44: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCorrect iterative walk; minor repeated allocation.
depth()andcontains_agent()each callchain(), reallocating aVec<String>(with per-node clones) just to count or search. Given delegation chains are expected to be short, this is unlikely to matter in practice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-sdk/src/delegation.rs` around lines 27 - 44, `depth()` and `contains_agent()` are rebuilding the full delegation chain via `chain()`, which causes avoidable `Vec<String>` allocations and per-node cloning. Update `depth()` and `contains_agent()` to walk the linked delegation structure directly using the same iterative traversal logic as `chain()` (or a shared internal iterator/helper) so counting and membership checks do not materialize the full chain. Keep the `chain()` method for callers that need the collected `Vec<String>`, but avoid using it inside `depth()` and `contains_agent()`.rsworkspace/crates/trogon-aauth-person/src/store/tests.rs (1)
7-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared claim fixtures to a common test-support module.
agent_claims()/resource_claims()are duplicated verbatim inpending/tests.rs(and similar minting helpers reappear inhttp/tests.rs). Consider hoisting these into a sharedtest_supportmodule for the crate so futureAgentClaims/ResourceClaimsfield changes only need a single update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-person/src/store/tests.rs` around lines 7 - 35, The claim fixture builders are duplicated across tests, so future AgentClaims/ResourceClaims changes will be easy to miss. Move agent_claims() and resource_claims() (and any similar minting helpers used by store tests) into a shared test_support module for the crate, then have store/tests.rs, pending/tests.rs, and http/tests.rs import and reuse those helpers instead of defining their own copies.rsworkspace/crates/trogon-aauth-person/src/http/mod.rs (2)
239-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an explicit mapping for
InteractionRequestTypehere.format!("{:?}", req.type_).to_lowercase()ties the logged value toDebugvariant names; a smallmatchorDisplayimpl keeps the log string aligned with the enum and avoids accidental drift on rename.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-person/src/http/mod.rs` at line 239, The request type logging in the HTTP module should not depend on Debug variant names from InteractionRequestType. Update the logic around the type_str assignment to use an explicit match or a Display implementation for InteractionRequestType so the logged string is intentionally mapped and stays stable if enum variants are renamed. Keep the change localized near the req.type_ handling in the http::mod::interaction request path.
141-151: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEncode the interaction payload explicitly
optionsis a generic list everywhere else; here it becomes a positional(url, code)payload. Add a dedicated structured field so clients don’t depend on array order.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-person/src/http/mod.rs` around lines 141 - 151, The PendingPhase::AwaitingResourceInteraction response is encoding the interaction data as a positional options array, which makes clients depend on element order. Update the PendingResponse shape used in http/mod.rs to include a dedicated structured field for the interaction payload, and populate it from the url and code values instead of packing them into options. Adjust the response construction in the AwaitingResourceInteraction arm so the payload is explicit and self-describing.rsworkspace/crates/trogon-aauth-as/src/error.rs (1)
92-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
PendingRequestIdforUnknownPendingRequest
Denied.reasoncan stay as a string, butUnknownPendingRequestshould carrycrate::pending::PendingRequestIdinstead ofStringso the pending-request identifier stays typed through the error boundary.rsworkspace/crates/trogon-aauth-as/src/error.rs:92-96🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-as/src/error.rs` around lines 92 - 96, `UnknownPendingRequest` is using an untyped String for the pending-request identifier, which breaks type safety across the error boundary. Update the `Error` enum in `error.rs` so `UnknownPendingRequest` carries `crate::pending::PendingRequestId` instead of `String`, and adjust the `#[error(...)]` formatting to display that typed id correctly; leave `Denied.reason` as a String.Source: Coding guidelines
rsworkspace/crates/trogon-aauth-as/src/test_support.rs (1)
63-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLong parameter lists in JWT-minting helpers.
mint_agent_jwt/mint_resource_jwt/mint_auth_jwt_raweach take 7-12 positional args with duplicated header/claims-building logic. Consider a small builder or shared claims-map helper if these grow further, but given this is test-only fixture code, it's not urgent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-as/src/test_support.rs` around lines 63 - 155, The JWT minting helpers mint_agent_jwt, mint_resource_jwt, and mint_auth_jwt_raw all have long positional parameter lists and duplicate header/claims assembly. Refactor them to reduce argument count and centralize shared token construction, for example by introducing a small builder or a shared claims helper that the three functions can reuse, while keeping the test-fixture behavior unchanged.rsworkspace/crates/trogon-aauth-person/src/agent/tests.rs (1)
15-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared JWK/PEM test fixtures.
The P-256 key generation, PEM export, and JWK-building logic here duplicates near-identical helpers in
trogon-aauth-person/src/mint/tests.rsandtrogon-aauth-person/tests/person_server_e2e.rs. The siblingtrogon-aauth-ascrate already has atest_support.rsmodule for this purpose — a similar crate-local test-support module here would reduce triplicated boilerplate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-person/src/agent/tests.rs` around lines 15 - 82, Extract the repeated P-256 key generation, PEM export, JWK/JWK set construction, and thumbprint helper logic from KeyFixture, key_fixture, and jkt_of into a crate-local test-support module, then update the tests in agent/tests.rs and the matching helpers in mint/tests.rs and person_server_e2e.rs to reuse that shared fixture code instead of duplicating it.rsworkspace/crates/trogon-jwks-publisher/src/provider/tests.rs (1)
138-141: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHardcoded private key PEM triggers secret-scanner noise.
Static analysis flagged this literal as a private key. Since sibling test files in this PR (e.g.,
agent/tests.rs,mint/tests.rs) already generate P-256 keys dynamically viaSigningKey::random(&mut OsRng), consider doing the same here to avoid recurring scanner false-positives and stay consistent with the rest of the PR.🔑 Proposed fix using dynamic key generation
-fn test_encoding_key() -> EncodingKey { - let pem = b"-----BEGIN PRIVATE KEY-----\nMIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgevZzL1gdAFr88hb2\nOF/2NxApJCzGCEDdfSp6VQO30hyhRANCAAQRWz+jn65BtOMvdyHKcvjBeBSDZH2r\n1RTwjmYSi9R/zpBnuQ4EiMnCqfMPWiZqB4QdbAd0E7oH50VpuZ1P087G\n-----END PRIVATE KEY-----\n"; - EncodingKey::from_ec_pem(pem).expect("test signing key") -} +fn test_encoding_key() -> EncodingKey { + let signing_key = p256::ecdsa::SigningKey::random(&mut rand_core::OsRng); + let pem = signing_key + .to_pkcs8_pem(pkcs8::LineEnding::LF) + .expect("encode pkcs8"); + EncodingKey::from_ec_pem(pem.as_bytes()).expect("test signing key") +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-jwks-publisher/src/provider/tests.rs` around lines 138 - 141, The hardcoded PEM literal in test_encoding_key is being flagged as a private-key secret, so replace the static test fixture with dynamically generated P-256 key material like the sibling test helpers in agent/tests.rs and mint/tests.rs. Update test_encoding_key to create a SigningKey with SigningKey::random(&mut OsRng), derive the corresponding EncodingKey from it, and keep the helper returning the same type so the tests remain consistent without embedding secrets.Source: Linters/SAST tools
rsworkspace/crates/trogon-aauth-as/Cargo.toml (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant duplicate
axumdependency.
axumis already declared in[dependencies]with identical spec; the[dev-dependencies]entry adds nothing since main dependencies are already available to tests/dev builds.🧹 Proposed cleanup
[dev-dependencies] -axum = { workspace = true } p256 = { version = "=0.13.2", features = ["ecdsa", "pkcs8"] }Also applies to: 29-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-as/Cargo.toml` at line 18, Remove the redundant axum entry from the dev-dependencies section in the Cargo.toml for trogon-aauth-as, since axum is already declared in the main dependencies with the same workspace spec. Keep the dependency only in [dependencies] and apply the same cleanup anywhere else the duplicate axum dev-dependency appears.rsworkspace/crates/trogon-jwks-publisher/src/provider.rs (1)
212-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
PersonServerUrldoc promises an HTTPS URL but the factory only checks non-empty.The type is documented as an "HTTPS URL (
psclaim)", yetnewaccepts any non-empty string (e.g."not a url"orhttp://...). Per the guideline that a type's factory must guarantee correctness at construction so invalid instances are unrepresentable, consider validating thehttps://scheme (mirroringrequire_https_issuerin the verify crate). The same applies toProviderIssuer(Lines 117-124), documented as an issuer URL.As per coding guidelines: "Each type's factory must guarantee correctness at construction—invalid instances should be unrepresentable."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-jwks-publisher/src/provider.rs` around lines 212 - 220, The factories for PersonServerUrl::new and ProviderIssuer::new only reject empty input, so they can construct invalid URL values despite the docs promising HTTPS URLs. Update these constructors to validate the parsed string is a proper https:// URL (not just non-empty), matching the behavior of require_https_issuer in the verify crate, and return the existing error type when the scheme or URL format is invalid.Source: Coding guidelines
rsworkspace/crates/trogon-aauth-person/src/server/tests.rs (1)
384-418: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't assert mission log accumulation despite its name.
mission_context_flows_into_policy_and_logonly checksis_active()/mission_ref().s256; it never asserts onmission.logafter the grant, even thoughPersonServer::apply_decisionappends aMissionLogEntryon the grant path. Consider fetching the mission after the grant and asserting the log entry was appended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-person/src/server/tests.rs` around lines 384 - 418, The test mission_context_flows_into_policy_and_log does not verify the log behavior implied by its name; after the grant is applied in PersonServer::apply_decision, fetch the mission and assert that mission.log contains the expected MissionLogEntry appended by the grant path. Keep the existing mission_ref and active-state checks, but add a direct assertion on the log accumulation after approve_mission/build_server flow so the test covers both policy and log propagation.rsworkspace/crates/a2a-gateway/src/runtime/dispatch.rs (1)
263-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor naming inconsistency: raw string literal vs.
ATTR_*constants.Sibling span fields elsewhere in this function use
ATTR_AGENT_SUBJECT,ATTR_ROUTING_OUTCOME,ATTR_CALLER_IDconstants;"aauth_agent_id"is a bare literal here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/a2a-gateway/src/runtime/dispatch.rs` around lines 263 - 265, The span field name in dispatch::dispatch is inconsistent with the other ATTR_* constants, since "aauth_agent_id" is a bare string literal while nearby fields use constants like ATTR_AGENT_SUBJECT, ATTR_ROUTING_OUTCOME, and ATTR_CALLER_ID. Replace the literal with a matching constant-style identifier (or introduce a dedicated constant alongside the others) and use it in the tracing::Span::current().record call so all span attribute keys are defined consistently in one place.rsworkspace/crates/trogon-aauth-sdk/src/exchange/client.rs (1)
181-190: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBody is serialized twice for signing vs. sending.
serde_json::to_vec(request)computes signing bytes separately from.json(request), which reqwest serializes again internally. Currently harmless sinceSignatureKeyOnlyHttpSignerignores thebodyargument, but it's wasted work and a latent correctness trap for a future signer that actually signs overbody— any serializer non-determinism between the two calls would desync the signature from the wire bytes.Also applies to: 267-271
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-sdk/src/exchange/client.rs` around lines 181 - 190, In the exchange client request path, the request body is serialized twice: once via serde_json::to_vec(request) for signing and again via .json(request) for sending, which can drift and wastes work. Update the request-building flow in client.rs around the sign(builder, &body) call so a single serialized body is produced and reused for both the signer and the HTTP request. Apply the same fix to the second duplicated exchange path noted in the comment, using the same exchange request-building symbols to keep signing bytes and wire bytes identical.rsworkspace/crates/trogon-aauth-as/src/pending.rs (1)
45-78: 🩺 Stability & Availability | 🔵 TrivialUnbounded pending store — abandoned
requirement=claimsentries never expire.
insertadds an entry that is only removed by a successfultake. A PS that receives a202but never POSTs the claims leaves the entry resident forever, so a stream of claims-required requests growsentrieswithout bound (memory pressure / DoS surface). This aligns with the documented single-node follow-up, but consider a TTL/eviction sweep or capacity bound before enabling in production.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-as/src/pending.rs` around lines 45 - 78, The PendingRequestStore in pending.rs can grow without bound because PendingRequest entries inserted via insert are only removed by take, so abandoned claims-required requests never expire. Add an eviction strategy to PendingRequestStore, such as a TTL with periodic cleanup or a bounded capacity/least-recently-used policy, and make sure new entries are aged out automatically even when ClaimsSubmission never arrives. Use the existing PendingRequestStore, insert, take, and entries symbols to implement the fix without changing the one-shot semantics of take.rsworkspace/crates/trogon-aauth-person/src/mission/tests.rs (1)
66-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't actually verify the "one-way" claim in its name.
Only a single
complete()call is exercised; nothing asserts that the transition is irreversible or idempotent (e.g. a secondcomplete()call, or thatappend_log/state mutation after completion is rejected if that's the intended invariant).✅ Suggested strengthening
fn complete_transitions_to_terminated_and_is_one_way() { let bytes = serde_json::to_vec(&blob()).unwrap(); let mut mission = Mission::approve(bytes, blob()); mission.complete(); assert!(!mission.is_active()); assert_eq!(mission.status, MissionStatus::Terminated); + // Calling complete() again (or any further activation) must not un-terminate the mission. + mission.complete(); + assert_eq!(mission.status, MissionStatus::Terminated); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-person/src/mission/tests.rs` around lines 66 - 72, The test name claims the transition is one-way, but it only checks a single call to Mission::complete. Strengthen complete_transitions_to_terminated_and_is_one_way by asserting the post-completion behavior is irreversible, such as calling Mission::complete a second time and verifying the status remains MissionStatus::Terminated and the mission stays inactive. If Mission exposes mutation helpers like append_log or similar state-changing methods, also verify they are rejected or have no effect after completion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rsworkspace/crates/trogon-aauth-as/src/http.rs`:
- Around line 70-85: The resume_endpoint handler currently accepts a
ClaimsSubmission without verifying the caller’s authenticated PS identity.
Update resume_endpoint to also extract Extension<PsIdentity>, then compare that
identity against the TrustedIssuer associated with the PendingRequest for the
PendingRequestId before calling server.resume_with_claims. If the PS does not
match, return an error response instead of proceeding, and keep the existing
outcome_response/error_response flow for the authorized path.
In `@rsworkspace/crates/trogon-aauth-as/src/policy.rs`:
- Around line 60-70: `Decision::Deny` currently stores denial cause as a raw
`String`, but the review expects a typed error value. Update the `Decision` enum
in `policy.rs` to replace `reason: String` with a typed `DenialReason` (prefer
an enum or newtype consistent with `GrantedScope` and `RequiredClaims`), then
adjust any constructors, matches, and token-endpoint error conversion code that
uses `Decision::Deny` so callers can inspect the denial cause without parsing
text.
In `@rsworkspace/crates/trogon-aauth-as/src/server.rs`:
- Around line 277-284: The pending ID generator in fresh_pending_id is
predictable because it derives the value from SystemTime::now().as_nanos(), so
replace it with a CSPRNG-backed identifier such as UUIDv4 or a 128-bit random
hex string. Update the fresh_pending_id helper in server.rs to generate an
unguessable value, and keep the returned string in the same format expected by
the pending-request store and Location URL usage.
In `@rsworkspace/crates/trogon-aauth-person/src/agent.rs`:
- Around line 54-60: The subagent check in agent verification is currently
swallowing parse/decode failures by using
parent_agent_of(agent_token).unwrap_or(None), which can let an invalid token
pass as a primary agent token. Update the logic around verify_agent and
parent_agent_of to fail closed: propagate the parent lookup error instead of
treating it as None, and map it through RequestVerificationError. Add a
RequestVerificationError variant that wraps ParentAgentError, and use it so any
token that cannot be parsed is rejected rather than accepted.
In `@rsworkspace/crates/trogon-aauth-person/src/error.rs`:
- Around line 79-97: The pending-request and mission error variants are using
bare String identifiers instead of the existing domain types. Update
PendingRequestError (and MissionNotFound if it shares the same pattern) to carry
MissionId or PendingId as appropriate, and adjust the Display/error annotations
plus any constructors/conversions in the surrounding error types to match. Use
the existing MissionId and PendingId re-exports from the crate so the typed
identifiers flow through the API consistently.
- Around line 118-124: Map
PersonServerError::Interaction(InteractionRelayError::UserUnreachable)
explicitly in token_endpoint_code() to the same UserUnreachable token endpoint
result as PersonServerError::UserUnreachable instead of falling through to
ServerError; then ensure http_status() continues to reflect that mapping through
token_endpoint_code(). Use the existing PersonServerError,
InteractionRelayError::UserUnreachable, token_endpoint_code(), and http_status()
symbols to locate the change.
In `@rsworkspace/crates/trogon-aauth-person/src/error/tests.rs`:
- Around line 1-64: Add test coverage in the existing PersonServerError error
tests for the InteractionRelayError::UserUnreachable case. In tests.rs, create a
case using
PersonServerError::Interaction(InteractionRelayError::UserUnreachable) and
assert that token_endpoint_code() resolves to
TokenEndpointError::UserUnreachable and http_status() resolves to 403, matching
the top-level UserUnreachable behavior. Use the existing PersonServerError,
InteractionRelayError, token_endpoint_code(), and http_status() symbols to
locate the mapping gap.
In `@rsworkspace/crates/trogon-aauth-person/src/http/mod.rs`:
- Around line 75-79: The HTTP error builder in error_response is leaking
internal verification details by passing err.to_string() into
ErrorResponse::with_detail. Change error_response so PersonServerError still
maps to the same status and wire_code, but the response detail is either a
generic client-safe auth message or omitted entirely for these failures. Keep
the fix localized to error_response and preserve the existing wire_code-based
error identification.
In `@rsworkspace/crates/trogon-aauth-person/src/mission.rs`:
- Around line 29-42: The Mission record is persisting a wire/input type
directly, since Mission currently stores MissionBlob and
PersonStateStore::insert_mission carries it through unchanged. Update the
mission storage path to convert MissionBlob into a validated domain type at the
boundary, and change Mission to hold only the domain representation needed for
runtime/storage instead of the raw wire type. Keep MissionBlob/mission log wire
bodies only at the deserialization edge, and ensure the persisted state uses
validated domain data only.
- Around line 15-27: MissionId currently exposes its inner String publicly,
which allows bypassing the invariant enforced by MissionId::from_blob_bytes.
Make the MissionId field private in the MissionId type, add a read-only accessor
or Deref if needed, and introduce an explicit constructor such as from_verified
for cases like server.rs that are already trusted. Ensure all construction paths
outside MissionId::from_blob_bytes go through these controlled APIs so invalid
MissionId values cannot be created directly.
In `@rsworkspace/crates/trogon-aauth-person/src/pending.rs`:
- Around line 21-41: PendingId generation in PendingId::generate uses
uuid_like(), which is predictable because it combines a process-global counter
and timestamp; replace it with a CSPRNG-backed unguessable ID. Update
uuid_like() to draw at least 128 bits of randomness from a secure source such as
rand/getrandom and encode it into a compact string form, while keeping
PendingId::generate as the single call site that constructs the ID.
In `@rsworkspace/crates/trogon-aauth-person/src/server.rs`:
- Around line 113-485: Store failures are being collapsed into
`PendingRequestError::NotFound`, which makes write and infrastructure errors
look like missing resources. Update `PersonServerError` and the error mappings
in `server.rs` so lookup methods still use `NotFound`, but `insert_pending`,
`update_pending`, `insert_mission`, `update_mission`, and similar write paths
return a dedicated store/error variant instead. Apply this consistently in
`persist`, `apply_decision`, `approve_mission`, `complete_mission`,
`append_mission_log`, `poll_pending`, `load_mission_context`, and the
clarification flow so callers can distinguish real absence from backend
failures.
- Around line 224-248: The interaction URL in `server.rs` is hardcoded to the
reserved `ps.invalid` domain inside the `PolicyDecision::NeedsInteraction`
handling, which breaks the `InteractionNotice` link in real deployments. Update
the URL construction to use the server’s actual issuer/base from `self.iss` (or
the appropriate existing issuer field on `PersonServer`) when building the
`InteractionNotice`, so the `InteractionNotice` URL resolves correctly while
keeping the same pending-id path and interaction flow.
- Around line 113-160: The pending correlation flow in `PersonServer::...` is
race-prone because `find_pending_by_correlation` and the later pending creation
path are separate operations, so concurrent requests can create duplicate
records. Update the lookup/create logic around
`PendingRequest::correlation_key_for`, `find_pending_by_correlation`, and the
pending insertion path to use a single atomic get-or-create mechanism, or
enforce a unique constraint/transaction in the store, so repeated requests
always reuse the same pending entry.
- Around line 406-421: The clarification round-trip in respond_to_clarification
is dropping the upstream delegation chain because it rebuilds VerifiedRequest
with upstream set to None. Preserve the upstream auth state from the pending
request when reconstructing VerifiedRequest, and make sure apply_decision still
receives that chain so the minted token continues to nest act correctly for
requests that arrived with an upstream_token.
In `@rsworkspace/crates/trogon-aauth-sdk/src/exchange/client.rs`:
- Around line 292-320: The polling loop in handle_response can spin indefinitely
when Retry-After is 0, so add a minimum sleep floor and a hard cap on
retries/overall polling duration. Update the handle_response flow to apply
core::DEFAULT_POLL_INTERVAL_SECS (or another shared minimum) whenever after_secs
is too small, and track poll iterations in ExchangeState or a local counter so
ExchangeAction::PollAfter cannot loop forever. Keep the fix localized to
handle_response, step, and the ExchangeState/outcome path so repeated
Pending/SlowDown responses are throttled and eventually fail cleanly.
In `@rsworkspace/crates/trogon-aauth-verify/src/jwks_http.rs`:
- Around line 255-263: The JWKS fetch path in the response body parsing logic
currently buffers the entire payload via response.bytes().await before checking
max_response_bytes, so the cap is enforced too late. Update the body handling in
jwks_http.rs to stream the response in chunks, accumulating into a buffer only
while tracking size and aborting as soon as the limit is exceeded; keep the
existing JSON parsing and error mapping in the JWK set parsing flow, especially
around the response.bytes()/from_slice logic.
- Around line 220-227: The issuer validation in require_https_issuer only checks
for an https:// prefix, so untrusted issuers can still point at arbitrary HTTPS
hosts. Tighten the validation in jwks_http.rs by rejecting private, loopback,
and link-local destinations (or otherwise enforcing a trusted allowlist) before
returning the normalized issuer, and ensure HttpJwksResolver::new in
a2a-gateway/src/runtime/aauth_env.rs only receives issuers that have passed this
trust check.
In `@rsworkspace/crates/trogon-identity-types/src/aauth/login.rs`:
- Around line 101-118: The `urldecode` helper in `login.rs` slices `&str` with
`&s[i + 1..i + 3]`, which can panic on malformed UTF-8 input and is reachable
through public `parse_query_string`. Update `urldecode` to read the two hex
characters directly from the `bytes` buffer (`bytes[i + 1]` and `bytes[i + 2]`)
before decoding with `u8::from_str_radix`, so decoding never depends on
potentially invalid string slicing.
---
Minor comments:
In `@rsworkspace/crates/a2a-gateway/src/runtime/aauth_env.rs`:
- Around line 234-239: The JWKS TTL parsing in well_known_resolver_from_env
currently accepts negative values from optional_i64, which causes
CachedJwksResolver to effectively disable caching; validate or clamp
ENV_AAUTH_JWKS_TTL_SECS to a non-negative value before passing it into
CachedJwksResolver::new(...).with_ttl_secs(...), and return an AAuthEnvError if
the env value is negative so the bad configuration is rejected early.
In `@rsworkspace/crates/trogon-aauth-as/src/subagent.rs`:
- Around line 36-45: The shape validation in parent_agent_of only consumes two
split segments, so it can accept a token with no signature segment even though
BadShape implies three dot-separated parts. Update the parsing in
parent_agent_of to explicitly verify that the JWT has exactly three segments
before decoding the payload, and keep the existing ParentAgentError::BadShape
path for any malformed input.
In `@rsworkspace/crates/trogon-aauth-sdk/src/subagent.rs`:
- Around line 54-56: Decode failures in parent_agent_of are being collapsed into
None, which hides malformed token errors and turns them into NotASubAgentToken.
Update parent_agent_of and the decode_payload flow in subagent.rs to preserve
and propagate the underlying decode error instead of discarding it, and make
build_subagent_token_request return the decode failure directly for
subagent_agent_jwt so malformed input surfaces as MalformedAgentToken. Keep the
error context attached in the typed error path rather than converting it to a
string or Option.
In `@rsworkspace/crates/trogon-aauth-verify/src/mission.rs`:
- Around line 13-16: The module docs contain a broken intra-doc link because
they refer to MissionClaim, but this crate actually uses MissionRef in this
context. Update the doc comment near extract_mission_claim and AuthClaims to
point the link at MissionRef so rustdoc resolves it correctly and the
documentation stays consistent with the types used here.
---
Nitpick comments:
In `@rsworkspace/crates/a2a-gateway/src/runtime/dispatch.rs`:
- Around line 263-265: The span field name in dispatch::dispatch is inconsistent
with the other ATTR_* constants, since "aauth_agent_id" is a bare string literal
while nearby fields use constants like ATTR_AGENT_SUBJECT, ATTR_ROUTING_OUTCOME,
and ATTR_CALLER_ID. Replace the literal with a matching constant-style
identifier (or introduce a dedicated constant alongside the others) and use it
in the tracing::Span::current().record call so all span attribute keys are
defined consistently in one place.
In `@rsworkspace/crates/trogon-aauth-as/Cargo.toml`:
- Line 18: Remove the redundant axum entry from the dev-dependencies section in
the Cargo.toml for trogon-aauth-as, since axum is already declared in the main
dependencies with the same workspace spec. Keep the dependency only in
[dependencies] and apply the same cleanup anywhere else the duplicate axum
dev-dependency appears.
In `@rsworkspace/crates/trogon-aauth-as/src/error.rs`:
- Around line 92-96: `UnknownPendingRequest` is using an untyped String for the
pending-request identifier, which breaks type safety across the error boundary.
Update the `Error` enum in `error.rs` so `UnknownPendingRequest` carries
`crate::pending::PendingRequestId` instead of `String`, and adjust the
`#[error(...)]` formatting to display that typed id correctly; leave
`Denied.reason` as a String.
In `@rsworkspace/crates/trogon-aauth-as/src/pending.rs`:
- Around line 45-78: The PendingRequestStore in pending.rs can grow without
bound because PendingRequest entries inserted via insert are only removed by
take, so abandoned claims-required requests never expire. Add an eviction
strategy to PendingRequestStore, such as a TTL with periodic cleanup or a
bounded capacity/least-recently-used policy, and make sure new entries are aged
out automatically even when ClaimsSubmission never arrives. Use the existing
PendingRequestStore, insert, take, and entries symbols to implement the fix
without changing the one-shot semantics of take.
In `@rsworkspace/crates/trogon-aauth-as/src/test_support.rs`:
- Around line 63-155: The JWT minting helpers mint_agent_jwt, mint_resource_jwt,
and mint_auth_jwt_raw all have long positional parameter lists and duplicate
header/claims assembly. Refactor them to reduce argument count and centralize
shared token construction, for example by introducing a small builder or a
shared claims helper that the three functions can reuse, while keeping the
test-fixture behavior unchanged.
In `@rsworkspace/crates/trogon-aauth-person/src/agent/tests.rs`:
- Around line 15-82: Extract the repeated P-256 key generation, PEM export,
JWK/JWK set construction, and thumbprint helper logic from KeyFixture,
key_fixture, and jkt_of into a crate-local test-support module, then update the
tests in agent/tests.rs and the matching helpers in mint/tests.rs and
person_server_e2e.rs to reuse that shared fixture code instead of duplicating
it.
In `@rsworkspace/crates/trogon-aauth-person/src/http/mod.rs`:
- Line 239: The request type logging in the HTTP module should not depend on
Debug variant names from InteractionRequestType. Update the logic around the
type_str assignment to use an explicit match or a Display implementation for
InteractionRequestType so the logged string is intentionally mapped and stays
stable if enum variants are renamed. Keep the change localized near the
req.type_ handling in the http::mod::interaction request path.
- Around line 141-151: The PendingPhase::AwaitingResourceInteraction response is
encoding the interaction data as a positional options array, which makes clients
depend on element order. Update the PendingResponse shape used in http/mod.rs to
include a dedicated structured field for the interaction payload, and populate
it from the url and code values instead of packing them into options. Adjust the
response construction in the AwaitingResourceInteraction arm so the payload is
explicit and self-describing.
In `@rsworkspace/crates/trogon-aauth-person/src/mission/tests.rs`:
- Around line 66-72: The test name claims the transition is one-way, but it only
checks a single call to Mission::complete. Strengthen
complete_transitions_to_terminated_and_is_one_way by asserting the
post-completion behavior is irreversible, such as calling Mission::complete a
second time and verifying the status remains MissionStatus::Terminated and the
mission stays inactive. If Mission exposes mutation helpers like append_log or
similar state-changing methods, also verify they are rejected or have no effect
after completion.
In `@rsworkspace/crates/trogon-aauth-person/src/server/tests.rs`:
- Around line 384-418: The test mission_context_flows_into_policy_and_log does
not verify the log behavior implied by its name; after the grant is applied in
PersonServer::apply_decision, fetch the mission and assert that mission.log
contains the expected MissionLogEntry appended by the grant path. Keep the
existing mission_ref and active-state checks, but add a direct assertion on the
log accumulation after approve_mission/build_server flow so the test covers both
policy and log propagation.
In `@rsworkspace/crates/trogon-aauth-person/src/store/tests.rs`:
- Around line 7-35: The claim fixture builders are duplicated across tests, so
future AgentClaims/ResourceClaims changes will be easy to miss. Move
agent_claims() and resource_claims() (and any similar minting helpers used by
store tests) into a shared test_support module for the crate, then have
store/tests.rs, pending/tests.rs, and http/tests.rs import and reuse those
helpers instead of defining their own copies.
In `@rsworkspace/crates/trogon-aauth-sdk/Cargo.toml`:
- Around line 26-34: Remove the redundant test-only re-declaration of
trogon-aauth-verify in Cargo.toml for trogon-aauth-sdk, since it is already
available through the existing [dependencies] entry with workspace = true. Keep
the dependency only once in the manifest and leave the other dev-dependencies
unchanged; use the trogon-aauth-verify and [dev-dependencies] entries to locate
the duplicate.
In `@rsworkspace/crates/trogon-aauth-sdk/src/delegation.rs`:
- Around line 27-44: `depth()` and `contains_agent()` are rebuilding the full
delegation chain via `chain()`, which causes avoidable `Vec<String>` allocations
and per-node cloning. Update `depth()` and `contains_agent()` to walk the linked
delegation structure directly using the same iterative traversal logic as
`chain()` (or a shared internal iterator/helper) so counting and membership
checks do not materialize the full chain. Keep the `chain()` method for callers
that need the collected `Vec<String>`, but avoid using it inside `depth()` and
`contains_agent()`.
In `@rsworkspace/crates/trogon-aauth-sdk/src/exchange/client.rs`:
- Around line 181-190: In the exchange client request path, the request body is
serialized twice: once via serde_json::to_vec(request) for signing and again via
.json(request) for sending, which can drift and wastes work. Update the
request-building flow in client.rs around the sign(builder, &body) call so a
single serialized body is produced and reused for both the signer and the HTTP
request. Apply the same fix to the second duplicated exchange path noted in the
comment, using the same exchange request-building symbols to keep signing bytes
and wire bytes identical.
In `@rsworkspace/crates/trogon-aauth-sdk/src/signer.rs`:
- Around line 171-182: The P-256 point-to-JWK coordinate extraction is
duplicated between public_jwk and jwk_from_ec_pkcs8_pem, and the two copies have
diverged in how missing coordinates are handled. Update the implementation so
both paths share the same logic or at least the same error behavior, and make
the publisher-side helper match public_jwk by returning an explicit
InvalidPublicKey-style error instead of silently using empty coordinates.
In `@rsworkspace/crates/trogon-identity-types/src/aauth/mission.rs`:
- Around line 106-129: MissionHeader::parse is duplicating header parsing logic
instead of using the shared split_header helper used by Requirement::parse, and
it currently aborts on any malformed segment rather than handling invalid parts
consistently. Update MissionHeader::parse in mission.rs to reuse split_header
(or factor both parsers onto a common helper) so parsing behavior matches the
existing tolerant approach, while still extracting approver and s256 from the
parsed segments.
In `@rsworkspace/crates/trogon-jwks-publisher/src/provider.rs`:
- Around line 212-220: The factories for PersonServerUrl::new and
ProviderIssuer::new only reject empty input, so they can construct invalid URL
values despite the docs promising HTTPS URLs. Update these constructors to
validate the parsed string is a proper https:// URL (not just non-empty),
matching the behavior of require_https_issuer in the verify crate, and return
the existing error type when the scheme or URL format is invalid.
In `@rsworkspace/crates/trogon-jwks-publisher/src/provider/tests.rs`:
- Around line 138-141: The hardcoded PEM literal in test_encoding_key is being
flagged as a private-key secret, so replace the static test fixture with
dynamically generated P-256 key material like the sibling test helpers in
agent/tests.rs and mint/tests.rs. Update test_encoding_key to create a
SigningKey with SigningKey::random(&mut OsRng), derive the corresponding
EncodingKey from it, and keep the helper returning the same type so the tests
remain consistent without embedding secrets.
In `@rsworkspace/crates/trogon-jwks-publisher/src/publisher.rs`:
- Around line 116-168: The builder and config currently store dwk names as raw
String keys, so validity is only checked at insertion time in
JwksPublisherConfigBuilder::with_jwk_set. Introduce a domain-specific
DwkFilename newtype/enum with a fallible constructor and use it in
JwksPublisherConfigBuilder::entries, JwksPublisherConfig::entries, and the
serve_dwk path extractor so only valid registered filenames can exist by type.
Keep the existing uniqueness and known-dwk checks at the conversion boundary,
and update all lookups/inserts to use the validated type instead of String.
- Around line 80-106: The JWK builder in jwk_from_ec_pkcs8_pem currently falls
back to empty x/y coordinates when the encoded point is missing them, which
hides a malformed key condition. Update this path to validate both point.x() and
point.y() explicitly and return a PublisherError (or the shared public-key error
used by the sibling helper) instead of unwrap_or_default, so invalid EC keys
fail fast rather than producing an empty-coordinate JWK. If possible, align this
logic with the equivalent public_jwk implementation to keep both code paths
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…g store and policy failures as not-found Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…ending ids unguessable Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…map relay user_unreachable Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
… details and validate mission blobs at the boundary Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…n get-or-create atomic Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…delegation context across resumes, and throttle sdk polling Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…ap, and decode login escapes without panicking Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
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)
rsworkspace/crates/trogon-aauth-as/src/server.rs (1)
104-108: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDon't swallow an invalid collapsed-mode
ps_iss.If
PsIssuer::new(ps_iss.clone())fails here, the server still starts and only rejects requests later as untrusted. This should fail during construction instead—ideally by storing a validated issuer value object inFederationMode::Collapsedor by makingAccessServer::new(...)return a typed init error. Based on learnings from the PR objectives, this regresses the "misconfigured auth layer fails to start" goal. As per coding guidelines: "Prefer domain-specific value objects over primitives" and "Each type's factory must guarantee correctness at construction—invalid instances should be unrepresentable."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-as/src/server.rs` around lines 104 - 108, The collapsed-mode `ps_iss` validation is being ignored in `AccessServer::new`, which lets startup succeed with an invalid issuer and fails later at request time. Make construction fail fast by validating `ps_iss` before building the server or, better, by moving the checked issuer into `FederationMode::Collapsed` as a validated value object so invalid state is unrepresentable. If validation still happens in `AccessServer::new`, return a typed init error instead of silently skipping `PsIssuer::new` failures and only calling `trust.trust` when the issuer is guaranteed valid.Source: Coding guidelines
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-aauth-as/src/policy.rs (1)
65-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
DenialReasonreject blank values.
DenialReason::new(...)currently accepts empty/whitespace strings even though this value is emitted verbatim in thedeniedresponse detail, so the value object still doesn't guarantee a valid domain instance. Make the constructor validate and return a typed error, or switch to a closed enum if the denial set is finite. As per coding guidelines: "Each type's factory must guarantee correctness at construction—invalid instances should be unrepresentable."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-as/src/policy.rs` around lines 65 - 69, DenialReason::new currently allows empty or whitespace-only strings, so the value object can be constructed in an invalid state. Update the DenialReason constructor in the DenialReason impl to validate the input, reject blank values, and return a typed error instead of always producing Self. If the set of reasons is meant to be finite, consider replacing the free-form string wrapper with a closed enum so invalid denial reasons are unrepresentable.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rsworkspace/crates/trogon-aauth-as/src/server.rs`:
- Around line 104-108: The collapsed-mode `ps_iss` validation is being ignored
in `AccessServer::new`, which lets startup succeed with an invalid issuer and
fails later at request time. Make construction fail fast by validating `ps_iss`
before building the server or, better, by moving the checked issuer into
`FederationMode::Collapsed` as a validated value object so invalid state is
unrepresentable. If validation still happens in `AccessServer::new`, return a
typed init error instead of silently skipping `PsIssuer::new` failures and only
calling `trust.trust` when the issuer is guaranteed valid.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-aauth-as/src/policy.rs`:
- Around line 65-69: DenialReason::new currently allows empty or whitespace-only
strings, so the value object can be constructed in an invalid state. Update the
DenialReason constructor in the DenialReason impl to validate the input, reject
blank values, and return a typed error instead of always producing Self. If the
set of reasons is meant to be finite, consider replacing the free-form string
wrapper with a closed enum so invalid denial reasons are unrepresentable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7730be2e-e39d-43bf-8e37-02371af24dde
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
rsworkspace/crates/a2a-gateway/src/aauth.rsrsworkspace/crates/a2a-gateway/src/runtime/aauth_env.rsrsworkspace/crates/a2a-gateway/src/runtime/aauth_env/tests.rsrsworkspace/crates/a2a-gateway/tests/aauth_roundtrip.rsrsworkspace/crates/a2a-gateway/tests/aauth_three_party_e2e.rsrsworkspace/crates/trogon-aauth-as/Cargo.tomlrsworkspace/crates/trogon-aauth-as/src/http.rsrsworkspace/crates/trogon-aauth-as/src/pending.rsrsworkspace/crates/trogon-aauth-as/src/policy.rsrsworkspace/crates/trogon-aauth-as/src/policy/tests.rsrsworkspace/crates/trogon-aauth-as/src/server.rsrsworkspace/crates/trogon-aauth-as/src/server/tests.rsrsworkspace/crates/trogon-aauth-person/Cargo.tomlrsworkspace/crates/trogon-aauth-person/src/agent.rsrsworkspace/crates/trogon-aauth-person/src/error.rsrsworkspace/crates/trogon-aauth-person/src/error/tests.rsrsworkspace/crates/trogon-aauth-person/src/http/mod.rsrsworkspace/crates/trogon-aauth-person/src/lib.rsrsworkspace/crates/trogon-aauth-person/src/mission.rsrsworkspace/crates/trogon-aauth-person/src/mission/tests.rsrsworkspace/crates/trogon-aauth-person/src/pending.rsrsworkspace/crates/trogon-aauth-person/src/pending/tests.rsrsworkspace/crates/trogon-aauth-person/src/server.rsrsworkspace/crates/trogon-aauth-person/src/server/tests.rsrsworkspace/crates/trogon-aauth-person/src/store.rsrsworkspace/crates/trogon-aauth-person/src/store/tests.rsrsworkspace/crates/trogon-aauth-sdk/src/exchange/client.rsrsworkspace/crates/trogon-aauth-sdk/src/exchange/client/tests.rsrsworkspace/crates/trogon-aauth-verify/Cargo.tomlrsworkspace/crates/trogon-aauth-verify/src/jwks_http.rsrsworkspace/crates/trogon-aauth-verify/src/jwks_http/tests.rsrsworkspace/crates/trogon-identity-types/src/aauth/login.rs
🚧 Files skipped from review as they are similar to previous changes (20)
- rsworkspace/crates/trogon-aauth-as/src/policy/tests.rs
- rsworkspace/crates/trogon-aauth-verify/Cargo.toml
- rsworkspace/crates/trogon-aauth-person/src/mission/tests.rs
- rsworkspace/crates/trogon-aauth-person/Cargo.toml
- rsworkspace/crates/trogon-aauth-person/src/lib.rs
- rsworkspace/crates/trogon-identity-types/src/aauth/login.rs
- rsworkspace/crates/trogon-aauth-as/Cargo.toml
- rsworkspace/crates/trogon-aauth-person/src/pending/tests.rs
- rsworkspace/crates/trogon-aauth-person/src/agent.rs
- rsworkspace/crates/trogon-aauth-as/src/server/tests.rs
- rsworkspace/crates/trogon-aauth-person/src/pending.rs
- rsworkspace/crates/trogon-aauth-as/src/http.rs
- rsworkspace/crates/a2a-gateway/src/runtime/aauth_env/tests.rs
- rsworkspace/crates/trogon-aauth-person/src/error.rs
- rsworkspace/crates/a2a-gateway/tests/aauth_three_party_e2e.rs
- rsworkspace/crates/a2a-gateway/src/runtime/aauth_env.rs
- rsworkspace/crates/trogon-aauth-sdk/src/exchange/client.rs
- rsworkspace/crates/trogon-aauth-person/src/http/mod.rs
- rsworkspace/crates/a2a-gateway/src/aauth.rs
- rsworkspace/crates/trogon-aauth-person/src/server.rs
…d resource tokens to the agent identifier Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…e workspace lint Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…s for the coverage gate Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…ing branches Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
… response verification Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
… token headers, and log deny encode failures Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 25f2e45. Configure here.
…metry key constant Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
rsworkspace/crates/trogon-aauth-sdk/src/exchange/challenge/tests.rs (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a runtime-generated key over a hardcoded test PEM.
Static analysis flags this as a private key. It's test-only fixture material so not a real leak, but other test fixtures in this PR (e.g.
SigningKey::random(&mut OsRng)introgon-aauth-person/src/server/tests.rs) generate keys at runtime instead of hardcoding PEM bytes, avoiding secret-scanner noise and one less magic constant to maintain.♻️ Suggested approach
-const P256_PEM: &[u8] = b"-----BEGIN PRIVATE KEY-----\n...==\n-----END PRIVATE KEY-----\n"; - -fn p256_jwk_for_test() -> jsonwebtoken::jwk::Jwk { - serde_json::from_value(serde_json::json!({ - "kty": "EC", "crv": "P-256", "kid": "k1", "alg": "ES256", "use": "sig", - "x": "EVs_o5-uQbTjL3chynL4wXgUg2R9q9UU8I5mEovUf84", - "y": "kGe5DgSIycKp8w9aJmoHhB1sB3QTugfnRWm5nU_TzsY" - })) - .unwrap() -} - -fn signed_resource_token(claims: &serde_json::Value) -> String { - let signing = jsonwebtoken::EncodingKey::from_ec_pem(P256_PEM).expect("signing key"); +fn signed_resource_token(claims: &serde_json::Value, signing_key: &p256::ecdsa::SigningKey) -> String { + let pem = signing_key.to_pkcs8_pem(pkcs8::LineEnding::LF).unwrap(); + let signing = jsonwebtoken::EncodingKey::from_ec_pem(pem.as_bytes()).expect("signing key"); let mut header = jsonwebtoken::Header::new(Algorithm::ES256); header.typ = Some(TYP_RESOURCE.into()); header.kid = Some("k1".into()); jsonwebtoken::encode(&header, claims, &signing).expect("encode") }(derive the matching JWK's
x/yfrom the generated verifying key, as done elsewhere in this codebase viaEncodedPoint).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-sdk/src/exchange/challenge/tests.rs` at line 14, Replace the hardcoded P256_PEM fixture in the challenge tests with a key generated at runtime, similar to the SigningKey::random(&mut OsRng) pattern used in trogon-aauth-person/src/server/tests.rs. Update the relevant challenge test setup to derive the corresponding JWK values from the generated verifying key using the existing EncodedPoint-based approach, so the test still exercises the same behavior without embedding private-key PEM bytes.Source: Linters/SAST tools
rsworkspace/crates/trogon-aauth-as/src/http/tests.rs (1)
25-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
ScopeSwitchedPolicywithin the same crate.An equivalent
ScopeSwitchedPolicy/decideimpl already exists introgon-aauth-as/src/policy/tests.rs(same crate, per graph context). Sincecrate::test_supportalready provides shared test fixtures used by this file, consider movingScopeSwitchedPolicythere so both test modules share one definition instead of two copies that can drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-as/src/http/tests.rs` around lines 25 - 52, There is a duplicated test-only ScopeSwitchedPolicy implementation in this module that already exists elsewhere in the same crate, so consolidate it into the shared crate::test_support fixtures instead of keeping two copies. Move the OrganizationPolicy implementation for ScopeSwitchedPolicy into the shared test support area and update the tests in http/tests.rs and the existing policy tests to import and use that single definition. Keep the decide and decide_with_claims behavior unchanged while removing the duplicate local struct/impl.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rsworkspace/crates/trogon-aauth-sdk/src/verify_response/tests.rs`:
- Around line 244-264: The test name is misleading because the scenario in
with_jwks_runs_claims_checks_after_signature_and_surfaces_audience_mismatch
actually varies own_agent_id and asserts VerifyResponseError::AgentMismatch, not
an audience failure. Rename the test to reflect the real condition under test
and keep the assertion aligned with the current behavior in
verify_auth_token_response_with_jwks so the name matches the error variant being
checked.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-aauth-as/src/http/tests.rs`:
- Around line 25-52: There is a duplicated test-only ScopeSwitchedPolicy
implementation in this module that already exists elsewhere in the same crate,
so consolidate it into the shared crate::test_support fixtures instead of
keeping two copies. Move the OrganizationPolicy implementation for
ScopeSwitchedPolicy into the shared test support area and update the tests in
http/tests.rs and the existing policy tests to import and use that single
definition. Keep the decide and decide_with_claims behavior unchanged while
removing the duplicate local struct/impl.
In `@rsworkspace/crates/trogon-aauth-sdk/src/exchange/challenge/tests.rs`:
- Line 14: Replace the hardcoded P256_PEM fixture in the challenge tests with a
key generated at runtime, similar to the SigningKey::random(&mut OsRng) pattern
used in trogon-aauth-person/src/server/tests.rs. Update the relevant challenge
test setup to derive the corresponding JWK values from the generated verifying
key using the existing EncodedPoint-based approach, so the test still exercises
the same behavior without embedding private-key PEM bytes.
🪄 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: fa5e11c7-cbf4-4f89-b0c0-a4d84d66140d
📒 Files selected for processing (35)
rsworkspace/crates/a2a-gateway/src/aauth.rsrsworkspace/crates/a2a-gateway/src/runtime/aauth_env.rsrsworkspace/crates/a2a-gateway/src/runtime/aauth_env/tests.rsrsworkspace/crates/a2a-gateway/src/runtime/dispatch.rsrsworkspace/crates/a2a-nats/src/gateway_ingress/tests.rsrsworkspace/crates/trogon-aauth-as/src/error.rsrsworkspace/crates/trogon-aauth-as/src/http.rsrsworkspace/crates/trogon-aauth-as/src/http/tests.rsrsworkspace/crates/trogon-aauth-as/src/policy/tests.rsrsworkspace/crates/trogon-aauth-as/src/verify.rsrsworkspace/crates/trogon-aauth-as/src/verify/tests.rsrsworkspace/crates/trogon-aauth-person/src/agent/tests.rsrsworkspace/crates/trogon-aauth-person/src/error/tests.rsrsworkspace/crates/trogon-aauth-person/src/http/mod.rsrsworkspace/crates/trogon-aauth-person/src/mission/tests.rsrsworkspace/crates/trogon-aauth-person/src/pending/tests.rsrsworkspace/crates/trogon-aauth-person/src/server/tests.rsrsworkspace/crates/trogon-aauth-sdk/src/exchange/challenge/tests.rsrsworkspace/crates/trogon-aauth-sdk/src/exchange/client.rsrsworkspace/crates/trogon-aauth-sdk/src/exchange/client/tests.rsrsworkspace/crates/trogon-aauth-sdk/src/exchange/core/tests.rsrsworkspace/crates/trogon-aauth-sdk/src/signer.rsrsworkspace/crates/trogon-aauth-sdk/src/tests.rsrsworkspace/crates/trogon-aauth-sdk/src/verify_response/tests.rsrsworkspace/crates/trogon-aauth-verify/src/delegation/tests.rsrsworkspace/crates/trogon-aauth-verify/src/http_pop/tests.rsrsworkspace/crates/trogon-aauth-verify/src/jwks_http/tests.rsrsworkspace/crates/trogon-aauth-verify/src/mission.rsrsworkspace/crates/trogon-aauth-verify/src/nats_pop.rsrsworkspace/crates/trogon-aauth-verify/src/nats_pop/tests.rsrsworkspace/crates/trogon-identity-types/src/aauth/headers.rsrsworkspace/crates/trogon-identity-types/src/aauth/login.rsrsworkspace/crates/trogon-identity-types/src/aauth/mission.rsrsworkspace/crates/trogon-identity-types/src/aauth/tests.rsrsworkspace/crates/trogon-jwks-publisher/src/publisher.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- rsworkspace/crates/trogon-aauth-person/src/mission/tests.rs
- rsworkspace/crates/a2a-nats/src/gateway_ingress/tests.rs
- rsworkspace/crates/trogon-aauth-as/src/policy/tests.rs
- rsworkspace/crates/trogon-aauth-verify/src/mission.rs
- rsworkspace/crates/trogon-aauth-as/src/http.rs
- rsworkspace/crates/trogon-aauth-as/src/verify.rs
- rsworkspace/crates/trogon-aauth-as/src/error.rs
- rsworkspace/crates/trogon-aauth-person/src/pending/tests.rs
- rsworkspace/crates/a2a-gateway/src/runtime/aauth_env/tests.rs
- rsworkspace/crates/trogon-aauth-sdk/src/signer.rs
- rsworkspace/crates/trogon-aauth-sdk/src/tests.rs
- rsworkspace/crates/trogon-aauth-sdk/src/exchange/client.rs
- rsworkspace/crates/trogon-aauth-person/src/http/mod.rs
- rsworkspace/crates/a2a-gateway/src/aauth.rs
- rsworkspace/crates/a2a-gateway/src/runtime/aauth_env.rs
- rsworkspace/crates/a2a-gateway/src/runtime/dispatch.rs
…smuggling and subagent identifier binding Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…e workspace lint Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…ons, and align a test name Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
rsworkspace/crates/trogon-aauth-sdk/src/delegation.rs (1)
41-43: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid building the full chain just to count/search it.
depth()andcontains_agent()both callchain(), cloning every agent string into aVec<String>before counting or scanning. An iterator over&Actnodes (no cloning, early-exit forcontains_agent) would be more efficient, though the current chains are short enough that this is unlikely to matter in practice.♻️ Possible refactor avoiding the Vec allocation
- fn contains_agent(&self, agent_id: &str) -> bool { - self.chain().iter().any(|agent| agent == agent_id) - } + fn contains_agent(&self, agent_id: &str) -> bool { + let mut current = Some(self); + while let Some(act) = current { + if act.agent == agent_id { + return true; + } + current = act.act.as_deref(); + } + false + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-sdk/src/delegation.rs` around lines 41 - 43, The `contains_agent` helper currently builds the full `chain()` Vec and clones every agent string before searching, which is unnecessary. Refactor `Delegation::contains_agent` (and the related `depth()` logic if convenient) to iterate directly over the underlying `Act` chain, so the lookup can short-circuit without allocating or cloning. Keep the behavior the same, but use an iterator-based traversal from the existing `Delegation`/`Act` structures instead of calling `chain()`.rsworkspace/crates/trogon-aauth-verify/src/jwks_http.rs (1)
274-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffError variant carries a formatted string, not a typed error.
JwksError::Transport(format!(...))mirrors the existing pattern elsewhere in this file, but per coding guidelines errors must be typed (struct/enum variants), neverString/format!(). A full fix means givingJwksError::Transportstructured fields (e.g.,iss: String, reason: TransportReason) across all call sites in this file, not just this arm.As per coding guidelines: "Errors must be typed—use structs or enums, never
Stringorformat!(). Every error type must implementDisplayandstd::error::Error."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-verify/src/jwks_http.rs` around lines 274 - 278, `JwksError::Transport` is still being constructed from a formatted string in `jwks_http.rs`, which violates the typed-error guideline. Update `JwksError::Transport` to carry structured data (for example `iss: String` plus a typed reason enum/struct) and implement `Display`/`Error` for the new error type. Then replace this arm and the other `JwksError::Transport(format!(...))` call sites in `jwks_http.rs` to construct the typed variant instead of embedding message text.Source: Coding guidelines
rsworkspace/crates/a2a-gateway/tests/aauth_roundtrip.rs (1)
655-672: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
ap_public_jwkconstruction block across the two new tests.The ~18-line block building the EC public
Jwkfromap_signing(point encoding,CommonParameters,AlgorithmParameters::EllipticCurve) is repeated verbatim in bothresolve_nats_denies_expired_auth_token_with_valid_popandresolve_nats_denies_mission_header_mismatching_claim, and likely elsewhere in this file given the number ofresolve_natstests added in this cohort. Extracting a smallfn ap_public_jwk_from(signing: &SigningKey, kid: &str) -> Jwkhelper (similar to the existingagent_fixturehelper in this same file) would remove the duplication.Also applies to: 729-746
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/a2a-gateway/tests/aauth_roundtrip.rs` around lines 655 - 672, The `ap_public_jwk` construction is duplicated across the new `resolve_nats_*` tests, so extract it into a small helper in `aauth_roundtrip.rs` (for example, an `ap_public_jwk_from` function that takes the signing key and key id) and reuse that helper in both `resolve_nats_denies_expired_auth_token_with_valid_pop` and `resolve_nats_denies_mission_header_mismatching_claim`, following the pattern of the existing `agent_fixture` helper to keep the EC/JWK setup in one place.rsworkspace/crates/trogon-aauth-person/src/http/tests.rs (1)
168-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
FailingStoreduplicates the identical test double already inserver/tests.rs.The
FailingStoreimpl here is byte-for-byte identical to the one atrsworkspace/crates/trogon-aauth-person/src/server/tests.rs:518-558(all ninePersonStateStoremethods returningStoreError("backend down".into())). Consider hoisting this into a shared test-support module (the siblingtrogon-aauth-ascrate already uses this pattern viatest_support.rs) so both test modules import one definition instead of maintaining two copies that can drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-aauth-person/src/http/tests.rs` around lines 168 - 204, The FailingStore test double is duplicated and should be shared instead of maintained in two places. Move the identical PersonStateStore failure stub used by the http tests and the server tests into a common test-support module for the trogon-aauth-person crate, then update the http::tests and server::tests modules to import and use that shared FailingStore definition so the implementations stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rsworkspace/crates/trogon-aauth-person/src/http/tests.rs`:
- Around line 729-744: The missing-mission case in the permission flow is
currently being treated as a generic server failure, so update the error mapping
in the request handlers for permission, interaction, and audit to translate
PersonServerError::MissionNotFound into a 4xx client error instead of routing it
through server_error. Use the relevant handler/error-conversion path in the HTTP
layer to distinguish absent or unknown mission refs from real persistence
failures, and update the
permission_endpoint_without_mission_returns_mission_not_found test to assert the
client-error status.
---
Nitpick comments:
In `@rsworkspace/crates/a2a-gateway/tests/aauth_roundtrip.rs`:
- Around line 655-672: The `ap_public_jwk` construction is duplicated across the
new `resolve_nats_*` tests, so extract it into a small helper in
`aauth_roundtrip.rs` (for example, an `ap_public_jwk_from` function that takes
the signing key and key id) and reuse that helper in both
`resolve_nats_denies_expired_auth_token_with_valid_pop` and
`resolve_nats_denies_mission_header_mismatching_claim`, following the pattern of
the existing `agent_fixture` helper to keep the EC/JWK setup in one place.
In `@rsworkspace/crates/trogon-aauth-person/src/http/tests.rs`:
- Around line 168-204: The FailingStore test double is duplicated and should be
shared instead of maintained in two places. Move the identical PersonStateStore
failure stub used by the http tests and the server tests into a common
test-support module for the trogon-aauth-person crate, then update the
http::tests and server::tests modules to import and use that shared FailingStore
definition so the implementations stay in sync.
In `@rsworkspace/crates/trogon-aauth-sdk/src/delegation.rs`:
- Around line 41-43: The `contains_agent` helper currently builds the full
`chain()` Vec and clones every agent string before searching, which is
unnecessary. Refactor `Delegation::contains_agent` (and the related `depth()`
logic if convenient) to iterate directly over the underlying `Act` chain, so the
lookup can short-circuit without allocating or cloning. Keep the behavior the
same, but use an iterator-based traversal from the existing `Delegation`/`Act`
structures instead of calling `chain()`.
In `@rsworkspace/crates/trogon-aauth-verify/src/jwks_http.rs`:
- Around line 274-278: `JwksError::Transport` is still being constructed from a
formatted string in `jwks_http.rs`, which violates the typed-error guideline.
Update `JwksError::Transport` to carry structured data (for example `iss:
String` plus a typed reason enum/struct) and implement `Display`/`Error` for the
new error type. Then replace this arm and the other
`JwksError::Transport(format!(...))` call sites in `jwks_http.rs` to construct
the typed variant instead of embedding message text.
🪄 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: 3152ffd3-ac95-4328-b559-7bef04a255a3
📒 Files selected for processing (44)
rsworkspace/crates/a2a-gateway/src/aauth/tests.rsrsworkspace/crates/a2a-gateway/src/runtime/aauth_env/tests.rsrsworkspace/crates/a2a-gateway/tests/aauth_roundtrip.rsrsworkspace/crates/a2a-nats/src/gateway_ingress/tests.rsrsworkspace/crates/trogon-aauth-as/src/error.rsrsworkspace/crates/trogon-aauth-as/src/http.rsrsworkspace/crates/trogon-aauth-as/src/server/tests.rsrsworkspace/crates/trogon-aauth-as/src/trust/tests.rsrsworkspace/crates/trogon-aauth-as/src/verify.rsrsworkspace/crates/trogon-aauth-as/src/verify/tests.rsrsworkspace/crates/trogon-aauth-person/src/agent.rsrsworkspace/crates/trogon-aauth-person/src/agent/tests.rsrsworkspace/crates/trogon-aauth-person/src/error.rsrsworkspace/crates/trogon-aauth-person/src/http/tests.rsrsworkspace/crates/trogon-aauth-person/src/server/tests.rsrsworkspace/crates/trogon-aauth-sdk/src/capabilities.rsrsworkspace/crates/trogon-aauth-sdk/src/capabilities/tests.rsrsworkspace/crates/trogon-aauth-sdk/src/delegation.rsrsworkspace/crates/trogon-aauth-sdk/src/delegation/tests.rsrsworkspace/crates/trogon-aauth-sdk/src/exchange/client.rsrsworkspace/crates/trogon-aauth-sdk/src/exchange/client/tests.rsrsworkspace/crates/trogon-aauth-sdk/src/verify_response/tests.rsrsworkspace/crates/trogon-aauth-verify/src/jwks_http.rsrsworkspace/crates/trogon-aauth-verify/src/jwks_http/tests.rsrsworkspace/crates/trogon-aauth-verify/src/mission/tests.rsrsworkspace/crates/trogon-aauth-verify/src/nats_pop.rsrsworkspace/crates/trogon-aauth-verify/src/nats_pop/tests.rsrsworkspace/crates/trogon-identity-types/src/aauth/delegation.rsrsworkspace/crates/trogon-identity-types/src/aauth/delegation/tests.rsrsworkspace/crates/trogon-identity-types/src/aauth/error.rsrsworkspace/crates/trogon-identity-types/src/aauth/error/tests.rsrsworkspace/crates/trogon-identity-types/src/aauth/federation.rsrsworkspace/crates/trogon-identity-types/src/aauth/federation/tests.rsrsworkspace/crates/trogon-identity-types/src/aauth/headers.rsrsworkspace/crates/trogon-identity-types/src/aauth/headers/tests.rsrsworkspace/crates/trogon-identity-types/src/aauth/login.rsrsworkspace/crates/trogon-identity-types/src/aauth/login/tests.rsrsworkspace/crates/trogon-identity-types/src/aauth/mission.rsrsworkspace/crates/trogon-identity-types/src/aauth/mission/tests.rsrsworkspace/crates/trogon-identity-types/src/aauth/person_server.rsrsworkspace/crates/trogon-identity-types/src/aauth/person_server/tests.rsrsworkspace/crates/trogon-jwks-publisher/src/provider/tests.rsrsworkspace/crates/trogon-jwks-publisher/src/publisher.rsrsworkspace/crates/trogon-jwks-publisher/src/publisher/tests.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- rsworkspace/crates/trogon-aauth-sdk/src/verify_response/tests.rs
- rsworkspace/crates/trogon-aauth-as/src/trust/tests.rs
- rsworkspace/crates/trogon-aauth-sdk/src/capabilities.rs
- rsworkspace/crates/trogon-aauth-as/src/http.rs
- rsworkspace/crates/trogon-aauth-as/src/error.rs
- rsworkspace/crates/trogon-aauth-person/src/agent/tests.rs
- rsworkspace/crates/a2a-gateway/src/aauth/tests.rs
- rsworkspace/crates/trogon-aauth-sdk/src/exchange/client/tests.rs
- rsworkspace/crates/trogon-aauth-as/src/server/tests.rs
- rsworkspace/crates/trogon-aauth-person/src/error.rs
- rsworkspace/crates/trogon-aauth-person/src/server/tests.rs
- rsworkspace/crates/trogon-aauth-person/src/agent.rs
- rsworkspace/crates/a2a-gateway/src/runtime/aauth_env/tests.rs
- rsworkspace/crates/trogon-aauth-as/src/verify/tests.rs
- rsworkspace/crates/trogon-aauth-as/src/verify.rs
- rsworkspace/crates/trogon-aauth-sdk/src/exchange/client.rs
…nvalid_request instead of server_error Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Known follow-ups, documented in code and ADR: in-memory replay and pending stores are single-node; the PS policy seam cannot yet assert a principal on grant; agent-only access without an auth token is deliberately allowed pending scope policy; SDK HTTP signing sends Signature-Key only, not the full RFC 9421 envelope.