feat(reborn): add product auth contracts - #3865
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the ironclaw_auth crate, establishing the product-facing authentication contracts for the Reborn project. The changes include core data structures for auth flows and credential accounts, secure interaction services, and a comprehensive suite of architecture and contract tests. Feedback focused on improving the robustness of the error handling by adding missing variants and ensuring the in-memory fake services correctly handle account updates and secret propagation as defined by the contracts.
…born-auth-product-contracts # Conflicts: # Cargo.toml
serrrfirat
left a comment
There was a problem hiding this comment.
Multi-agent code review completed for 186518ae3907a3e9510749a6100c86e73b720940.
Reviewers: security, bugs, performance/concurrency, tests, conventions. Validation run locally on the PR worktree:
cargo test -p ironclaw_authcargo test -p ironclaw_architecture reborn_product_auth_contract_stays_reborn_nativecargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_hold
Summary: I found several Medium contract issues around terminal state preservation, PKCE binding, authority-bearing secret handles on serialized account records, label-based account overwrite semantics, and typed-contract conformance. No Critical/High findings were identified.
serrrfirat
left a comment
There was a problem hiding this comment.
Multi-agent review completed for d9284e4636e5c16940a0ac62c4542c2d84079619.
Findings: 5 Medium, 3 Low. I am leaving this as a COMMENT rather than REQUEST_CHANGES because there are no Critical/High findings under the skill policy, but the Medium items are worth addressing before this contract becomes a dependency for production wiring.
Validation I ran:
cargo test -p ironclaw_auth --target-dir /tmp/ironclaw-target-pr3865passed.cargo test -p ironclaw_architecture reborn_product_auth_contract_stays_reborn_native --target-dir /tmp/ironclaw-target-pr3865passed.cargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_hold --target-dir /tmp/ironclaw-target-pr3865passed.git diff --check 759fe6f6f0d35dcadcb64c961b9b6499388b3543 d9284e4636e5c16940a0ac62c4542c2d84079619failed on trailing whitespace indocs/reborn/contracts/auth-product.mdlines 3-4.
Reviewer coverage: security, bugs, performance/concurrency, tests, conventions. Performance/concurrency returned no findings.
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review Summary: 6 findings (1 Medium, 2 Low, 2 Low, 1 Nit). No Critical/High. No blocking issues.
Findings by Category:
Security (1 Medium)
- Pattern-based forbidden checks miss wrapped types (conf: 75)
Theforbiddenpatterns inForbiddenRebornAuthUsematch literal string syntax only (e.g.,"access_token: String"). A newtype wrapper likestruct SecretToken { inner: String }bypasses all checks. CLAUDE.md says raw tokens must not enter serializable records — the spirit is preventing value leakage, not just string literals.
Fix: Add type-based enforcement (derive a marker trait for forbidden types, or scan for field types against a list).
Anchor: crates/ironclaw_auth/AGENTS.md:14
File: crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs, lines withForbiddenRebornAuthUsestruct
Bugs (1 Medium, 1 Low)
-
update_statusallows arbitrary state transitions (conf: 65)
Any status maps to any other status with no enforcement. CLAUDE.md says "Fakes should fail closed and model important state transitions closely enough." No transition table, no prevention of re-activation of Canceled flows.
Fix: Add a state transition allowlist.
File: crates/ironclaw_auth/src/fakes.rs,update_statusmethod -
validate_new_credential_account— incomplete validation (conf: 60)
Only checksExtensionOwnedrequiresowner_extension. Missing validation for empty scopes, label length, status validity for new accounts.
Fix: Add validation for empty scopes and label length.
File: crates/ironclaw_auth/src/fakes.rs,validate_new_credential_account
Conventions (1 Nit)
rust-version = "1.92"(conf: 80)
Verify this version exists and matches the workspace's Rust MSRV. If the project uses edition 2024, the version should be aligned with the workspace Cargo.toml.
File: crates/ironclaw_auth/Cargo.toml:4
Performance (1 Low)
- O(n*m) string scanning in architecture guardrail (conf: 60)
collect_forbidden_reborn_auth_usesreads every .rs file and doesline.contains(pattern)for each of ~10 patterns. Current cost is manageable (~10K comparisons) but doesn't scale to large codebases.
Fix: Use grep-based or Aho-Corasick multi-pattern matching.
File: crates/ironclaw_architecture/src/tests/reborn_dependency_boundaries.rs,collect_forbidden_reborn_auth_uses
Positive Notes
- Clean separation of concerns across modules (flow, credential, cleanup, provider, interaction)
- Strong newtypes in ids.rs prevent type confusion
- Proper secret redaction in Debug impls and projections
- Scope validation consistent across all service methods
- Architecture guardrails are comprehensive and well-documented
- Error codes are stable and sanitized for wire consumption
| This comment supersedes review 4346615705 which was posted prematurely before multi-agent review completed. A corrected review will follow. |
|
Addressed the active review threads in Covered:
Validation:
Note: broad |
serrrfirat
left a comment
There was a problem hiding this comment.
Multi-agent review completed for 881d3f53a789c418eb5eb9f43732fc9426ea7d9f.
Findings: 5 Medium, 1 Low. No Critical/High findings, so this is a COMMENT rather than REQUEST_CHANGES under the review policy.
Reviewer coverage: security, bugs, performance/concurrency, tests, conventions. Performance/concurrency returned no findings.
Validation I ran:
git diff --check 759fe6f6f0d35dcadcb64c961b9b6499388b3543 881d3f53a789c418eb5eb9f43732fc9426ea7d9fpassed.cargo test -p ironclaw_auth --target-dir /tmp/ironclaw-target-pr3865-forcepassed.cargo test -p ironclaw_architecture reborn_product_auth_contract_stays_reborn_native --target-dir /tmp/ironclaw-target-pr3865-forcepassed.cargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_hold --target-dir /tmp/ironclaw-target-pr3865-forcepassed.
|
Addressed the remaining review feedback in Covered:
Validation:
All open review threads are resolved. |
* feat(reborn): add product auth contracts * fix(reborn): address auth product review gaps * fix(auth): address serrrfirat review — harden auth contracts (nearai#3865) * fix(auth): address review feedback — tighten contracts (nearai#3865) * refactor(auth): simplify product auth contracts (nearai#3865) * fix(auth): address review status transitions (nearai#3865) --------- Co-authored-by: serrrfirat <f@nuff.tech> Co-authored-by: Henry Park <henrypark133@gmail.com>
Summary
ironclaw_authworkspace crate for Reborn product-facing auth contracts and fake services.Change Type
Linked Issue
Closes #3810
Related #3289
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo buildcargo test -p ironclaw_auth,cargo test -p ironclaw_architecture reborn_product_auth_contract_stays_reborn_native,cargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_holdcargo test --features integrationif database-backed or integration behavior changedreview-prorpr-shepherd --fixwas run before requesting reviewAdditional validation:
cargo clippy -p ironclaw_auth --all-targets -- -D warningsgit diff --checkSecurity Impact
Yes. This adds product-auth contract vocabulary and guardrails for OAuth/manual-token/credential-account flows. It improves the Reborn boundary by requiring raw OAuth code/PKCE material to stay in one-shot non-serializable provider inputs, keeping stored/projected records to ids, hashes, handles, statuses, and redacted metadata. No production listener, provider HTTP client, durable secret storage, runtime credential injection, database schema, file access, or sandbox policy is added in this slice.
Database Impact
None. This slice adds contracts, fakes, docs, and architecture tests only; no migrations or persistence backend changes.
Blast Radius
Limited to the new
ironclaw_authcrate, Reborn contract docs, workspace crate maps, and architecture boundary tests. Existing v1 auth, gateway, extension, and secret-store behavior is not wired through this crate.Rollback Plan
Revert this PR to remove the
ironclaw_authcrate, product-auth contract doc, workspace registration, and architecture guardrails. No data migration or runtime rollback is required.Review Follow-Through
Reviewer judgment requested on the contract vocabulary before production wiring begins, especially the auth-flow/credential-account split, continuation shape, cleanup semantics, and the hard rule that Reborn auth code paths must remain separate from v1 routes, pending maps, extension manager authority, and v1 secret stores.
Review track: C