test(auth): shared OAuth-flow conformance suite over fake and durable AuthFlowManager - #6114
Conversation
… AuthFlowManager The in-memory fake (ironclaw_auth) and the durable FilesystemAuthProductServices (ironclaw_reborn_composition) previously had disjoint test suites, so behavioral divergence between them was structurally undetectable — their agreement was coincidence, not contract. Found while pinning the #6105 T4 replay arm: the two impls DO agree today (claim is replay-idempotent on terminal flows, complete is fail-closed), but each hand-rolls its terminal-idempotency sets and expiry write-back separately from the shared validation helpers. Adds ironclaw_auth::conformance — an observable-behavior state-machine suite over &dyn AuthFlowManager (happy completion + both replay arms, expiry write-back, cancel, unknown flow, state-hash mismatch not burning the flow) — invoked from both tiers: - fake: auth_product_contract/oauth_flow_contract.rs - durable: tests/integration/oauth_connect.rs over the composed OAuthProductAuthTestBundle flow_manager() The suite drives pre-exchanged outcomes, so no token-exchange egress is involved; the exchange leg keeps its existing coverage. Related: #4202 (crash-window callback cleanup, not covered here), #5617 (same fakes-only failure class in the identity crate). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
size: L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a feature-gated OAuth callback conformance harness covering replay, expiry, cancellation, unknown flows, and state-hash fencing. The harness runs against both in-memory and durable flow managers through new integration tests. ChangesOAuth callback conformance
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Conformance as Conformance harness
participant InMemory as InMemoryAuthProductServices
participant Durable as Durable flow manager
participant Flow as OAuth flow record
Conformance->>InMemory: Run callback conformance cases
InMemory->>Flow: Claim, complete, cancel, and read flow
Flow-->>InMemory: Return lifecycle state and errors
InMemory-->>Conformance: Return conformance results
Conformance->>Durable: Run callback conformance cases
Durable->>Flow: Persist and read lifecycle state
Flow-->>Durable: Return lifecycle state and errors
Durable-->>Conformance: Return conformance results
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a shared OAuth-flow state-machine conformance suite to ensure that both the in-memory fake and durable filesystem implementations of AuthFlowManager satisfy the same behavioral contracts. It integrates this suite into the test suites of both implementations. The feedback suggests increasing the expiration offset in the expired flow test from 1 second to 10 seconds to prevent potential test flakiness on backends with lower timestamp precision.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| scope, | ||
| provider, | ||
| tag, | ||
| Utc::now() - Duration::seconds(1), |
There was a problem hiding this comment.
Using a very short expiration offset like Duration::seconds(1) can lead to flaky tests on durable implementations (such as databases or filesystems) that truncate timestamps to second precision, or during extremely fast test execution where the clock hasn't advanced. Increasing this to a larger offset (e.g., Duration::seconds(10)) ensures the flow is reliably treated as expired across all backends.
| Utc::now() - Duration::seconds(1), | |
| Utc::now() - Duration::seconds(10), |
There was a problem hiding this comment.
Applied in 966e7f3 — widened the expired-flow offset from Duration::seconds(1) to Duration::seconds(10) so second-precision timestamp truncation on the durable FilesystemAuthProductServices backend can't leave the flow borderline-unexpired. Thanks — good catch for the durable tier.
(The file also moved in this push: crates/ironclaw_auth/src/conformance.rs → crates/ironclaw_auth/src/test_support/conformance.rs, now feature-gated so its panics don't ship in production binaries.)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/integration/oauth_connect.rs`:
- Around line 144-163: Update tests/integration/coverage-floor.toml to record
the intentional coverage increase from
durable_flow_manager_satisfies_shared_oauth_flow_conformance and satisfy the
existing tests/integration/**/*.rs coverage-floor requirement. Preserve the
established floor-file format and adjust only the affected integration coverage
entries.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0c7b7183-2790-4a0f-bc09-01b7a16e5864
📒 Files selected for processing (4)
crates/ironclaw_auth/src/conformance.rscrates/ironclaw_auth/src/lib.rscrates/ironclaw_auth/tests/auth_product_contract/oauth_flow_contract.rstests/integration/oauth_connect.rs
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.69% — 305795 / 356859 lines Per-crate breakdown (63 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
|
🚅 Deployed to the ironclaw-pr-6114 environment in ironclaw-ci-preview
|
…anics gate (#6114 CI) The shared OAuth-flow conformance suite landed as an ungated `pub mod conformance;`, so its `.expect()`/`assert!`/`panic!` calls compiled into production binaries and the "No panics in production code" CI gate failed (cascading into the aggregate "Code Style" check). It was modeled on `fakes.rs` living "unconditionally in the lib" — but `fakes.rs` is panic-free (returns `Result`); a panic-on-violation assertion harness must not ship in production. Move it under a feature-gated `test_support` module — the repo's sanctioned pattern (ironclaw_agent_loop, ironclaw_product_adapters, …) and exactly why `check_no_panics.py` path-exempts `src/test_support/**` ("ships zero bytes in production"): - relocate `src/conformance.rs` -> `src/test_support/conformance.rs`, gated behind `#[cfg(any(test, feature = "test-support"))]`; - add the `test-support` feature + a self dev-dependency so the crate's own `tests/` reach it, and a root `[dev-dependencies]` `ironclaw_auth` with the feature so `reborn_integration_oauth_connect` reaches it (tests only — release binaries stay clean under resolver 2); - update both callers to `ironclaw_auth::test_support::conformance::…`. Also address the review nit: widen the expired-flow offset from 1s to 10s so second-precision timestamp truncation on the durable backend can't flake (gemini-code-assist). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
scope: dependencies
|
Pushed Root cause: the new conformance suite landed as an ungated Fix: moved it under a feature-gated
Also widened the expired-flow offset 1s→10s (gemini nit). Verified locally: |
This branch was successfully deployed
1 participant
Summary
Closes the fake↔durable conformance gap in product-auth OAuth flows, found while pinning the #6105 T4 replay arm (PR #6113): the in-memory
InMemoryAuthProductServicesfake and the durableFilesystemAuthProductServiceshad disjoint test suites, so any behavioral divergence between them was structurally undetectable.Investigating the apparent replay divergence showed the two implementations actually agree at every
AuthFlowManagermethod —claim_oauth_callbackis replay-idempotent on terminal flows in both,complete_oauth_callbackis fail-closed (FlowAlreadyTerminal) in both via the sharedprepare_callback_flow. What differed in #6113's observation was the call path (wrapper-levelhandle_oauth_callbackshort-circuits at the idempotent claim), not the implementations. But that agreement was a coincidence of two suites: each impl hand-rolls its terminal-idempotency sets and expiry write-back separately from the shared validation helpers, so they could drift with nothing failing.What's in it
ironclaw_auth::conformance— a shared, observable-behavior state-machine suite over&dyn AuthFlowManager(lives unconditionally in the lib next tofakes.rs, per the crate's "auth contracts and fake services" charter). Cases:UnknownOrExpiredFlowand the record is written back terminalExpired;UnknownOrExpiredFlow;CrossScopeDeniedwithout burning the flow (the genuine callback still completes).Both implementations invoke it:
crates/ironclaw_auth/tests/auth_product_contract/oauth_flow_contract.rstests/integration/oauth_connect.rsover the composedOAuthProductAuthTestBundle'sflow_manager()(realFilesystemAuthProductServices; the suite drives pre-exchanged outcomes, so no token-exchange egress — that leg keeps its existing coverage in the same file)Not covered here
ironclaw_reborn_identity— tracked by ironclaw_reborn_identity: login seam (OAuth route → WebuiUserDirectory → resolver) is tested only with fakes on both sides #5617.Testing
cargo test -p ironclaw_auth— 111 passed (incl. the new fake-tier conformance invocation)cargo test --test reborn_integration_oauth_connect— 3 passed (incl. the durable-tier invocation)cargo clippy -p ironclaw_auth --all-targets --all-featuresandcargo clippy --test reborn_integration_oauth_connect --all-features— clean;cargo fmtclean🤖 Generated with Claude Code