feat(reborn): wire product auth composition seam - #3878
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces RebornProductAuthServices as a centralized composition seam for authentication within the ironclaw_reborn_composition crate. Key changes include the integration of the ironclaw_auth dependency, updates to the service factory and build inputs to support auth services across local-dev and production profiles, and the addition of readiness tracking for the auth facade. Furthermore, architectural tests were added to enforce dependency boundaries, and a new test ensures that sensitive tokens are redacted during manual-token submission. Review feedback focused on documenting the restrictive trait bounds in the from_shared constructor, maintaining consistency in readiness flag logic across different build profiles, and improving error messaging in test helpers by using .expect().
67a43ef to
d70a260
Compare
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Wire RebornProductAuthServices composition seam into service readiness with coverage, docs, and architecture guardrail updates.
Stats: 5 findings (from 5 raw, 5 after dedup) across 4 files. Reviewers run: security, bugs, performance, tests, conventions, design. Reviewers failed: none. Body-only: 0
Tests
-
Medium — RebornProductAuthServices::new public constructor has no direct test (
crates/ironclaw_reborn_composition/src/auth.rs:46-62, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/auth.rs:46
The public constructor taking six separate trait-object Arcs is never called directly in any test. The doc comment states production should prefer this over from_shared, but no test exercises it with distinct mock implementations. -
Medium — RebornProductAuthServices::from_shared has no test with a custom multi-trait impl (
crates/ironclaw_reborn_composition/src/auth.rs:70-95, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/auth.rs:70
from_shared is only exercised indirectly via local_dev_in_memory() with InMemoryAuthProductServices. No test verifies that from_shared correctly clones the Arc for each trait slot when given a custom type implementing all six traits. -
Medium — RebornBuildInput::with_product_auth_services builder method has no test (
crates/ironclaw_reborn_composition/src/input.rs:171-176, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/input.rs:171
The public builder method for injecting custom product-auth services into production builds is never exercised. No test verifies that a production build with injected services reports product_auth readiness as true and returns the injected bundle. -
Low — Production build readiness with product_auth=false is not asserted in existing tests (
crates/ironclaw_reborn_composition/src/factory.rs:519-527, confidence 50) — anchor: crates/ironclaw_reborn_composition/src/factory.rs:522
Existing production tests do not assert that readiness.facades.product_auth is false when no auth services are injected, nor that services.product_auth is None. -
Low — collect_forbidden_reborn_auth_file_uses helper has no dedicated test (
crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs:2262-2283, confidence 50) — anchor: crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs:2262
The new helper uses .expect() on file read and iterates forbidden patterns. No test verifies it correctly detects a violation when auth.rs contains a forbidden pattern, or that it handles a missing file path gracefully.
d70a260 to
78094bf
Compare
* feat(reborn): wire product auth composition seam * fix(reborn): address auth composition review gaps
Summary
RebornProductAuthServicesas the single Reborn composition seam for product auth flows, secure manual-token interactions, credential setup/accounts, provider exchange, and cleanup.build_reborn_servicesreadiness: local-dev gets the in-memory Reborn auth implementation, while production only reports product-auth ready when a Reborn-native bundle is explicitly injected.Change Type
Linked Issue
Closes #3811
Related #3289
Stacked on #3865 (
feat/reborn-auth-product-contracts). After #3865 lands, this PR should be retargeted toreborn-integration.Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo buildcargo test -p ironclaw_reborn_composition --locked,cargo test -p ironclaw_auth --locked,cargo test -p ironclaw_architecture reborn_product_auth_contract_stays_reborn_native --locked,cargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_hold --locked,cargo test -p ironclaw_architecture no_substrate_crate_depends_on_composition_root --lockedcargo test --features integrationif database-backed or integration behavior changedreview-prorpr-shepherd --fixwas run before requesting reviewAdditional validation:
cargo clippy -p ironclaw_reborn_composition --all-targets -- -D warningscargo check -p ironclaw_reborn_composition --features libsql --lockedcargo check -p ironclaw_reborn_composition --features postgres --lockedgit diff --checkSecurity Impact
Yes. This adds the Reborn-native product-auth composition seam. It keeps callers on trait-shaped product-auth ports and documents that production must inject durable Reborn auth services explicitly instead of falling back to V1 routes, V1 pending maps, V1
ExtensionManager, V1 secret stores, or route-local raw HTTP clients. No production OAuth route, listener, raw provider transport, durable secret storage, token material handling, runtime credential injection, or database schema is added in this slice.Database Impact
None. This wires composition/readiness and tests only; durable product-auth storage remains future substrate work.
Blast Radius
Limited to
ironclaw_reborn_composition, the product-auth contract docs, and architecture guardrails. Existing V1 auth behavior is untouched. Local-dev now reports product-auth readiness through the in-memory Reborn auth implementation; production reports it only when explicitly injected.Rollback Plan
Revert this PR to remove the composition bundle, readiness flag, docs, and guardrail updates. No data migration or runtime cleanup is required.
Review Follow-Through
Reviewer judgment requested on the exact composition surface and readiness semantics. #3094 remains separate: blocked run-state approval/auth gate listing, user decisions, and trusted resume should consume this auth boundary rather than adding a second auth model.
Review track: C