fix(host-runtime): auth-gate fingerprint includes setup; + #6299 CodeRabbit cleanups - #6303
Conversation
…cleanups) Follow-up to #6299 (capability-result collapse). Test-only maintainability cleanups CodeRabbit flagged; no production behavior change. - Extract the duplicated `assert_recoverable_failure` test helper (byte-identical in `local_dev/outbound_delivery.rs` and `local_dev/project_create.rs`) into one `#[cfg(test)] pub(crate)` helper in `local_dev.rs`; both submodule test blocks import it. - Extract a shared `approval_gate_ref(&Resolution) -> LoopGateRef` helper in `ironclaw_runner/tests/hooks_integration.rs`, replacing the same `Resolution::Blocked(Blocked::Approval) → origin() → LoopGateRef` shape repeated 6× (centralizing the panic/expect messages). - Fix a stale doc comment on `RebornCapabilityBackend::gate_record_store` in `tests/integration/support/group.rs`: the host-runtime arm always returns `Some` (`HostRuntimeCapabilityHarness::gate_record_store`), so only the `Recording` backend yields `None`. CodeRabbit's Cargo.toml feature-gate nit does not apply: `ironclaw_capabilities` has no `[features]` section, and the consumed `FilesystemReplayPayloadStore` is a production type, so the bare dev-dependency is correct. Verified: `cargo test -p ironclaw_reborn_composition --lib local_dev` (236 passed) and `cargo test -p ironclaw_runner --test hooks_integration` (49 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…int (#6299 IronLoop) The deterministic auth-gate id (`stable_auth_gate_id`) hashed the capability, required secret handles, and each credential requirement's provider/requester/`provider_scopes` — but NOT its `setup` (`RuntimeCredentialAccountSetup`, including OAuth setup scopes). Two auth requirements that agree on everything else but differ in setup (e.g. a ManualToken record vs a later OAuth or Pairing record, or two OAuth setups with different setup scopes) therefore derived the SAME `for_auth_gate` key. At the loop-host persist seam the write-once gate-record store then reports `GateRecordAlreadyExists` and — treating a deterministic-key collision as a benign byte-identical re-raise — keeps the STALE record. The runner reloads it and renders the wrong authentication flow (`credential_requirements` from the first, obsolete record). Fix: fold a canonical `setup` token into the fingerprint via `stable_setup_token` (exhaustive match over `RuntimeCredentialAccountSetup`, OAuth setup scopes sorted to match the `provider_scopes` canonicalization). Distinct setups now derive distinct keys and never reach the collision branch with a stale record; the "byte-identical" assumption at `capability_port.rs` is now sound (comment updated to say why). Regression test `auth_required_outcome_changes_gate_when_only_setup_changes` holds provider/requester/`provider_scopes` fixed and varies ONLY `setup` (ManualToken vs OAuth vs Pairing, plus two OAuth setups with different setup scopes); it fails before this fix (all share one gate id) and passes after. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
|
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 selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesAuth gate identity
Test helper consolidation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
github-actions
Bot
added
size: M
![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 resolves a collision issue in stable_auth_gate_id by incorporating the credential-account setup into the fingerprint token generation, and adds corresponding regression tests. It also refactors test helpers across multiple files to reduce code duplication, such as assert_recoverable_failure and approval_gate_ref. Feedback is provided to optimize stable_setup_token by returning std::borrow::Cow<'static, str> instead of String to avoid unnecessary heap allocations for static variants.
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.
| fn stable_setup_token(setup: &ironclaw_host_api::RuntimeCredentialAccountSetup) -> String { | ||
| use ironclaw_host_api::RuntimeCredentialAccountSetup as Setup; | ||
| match setup { | ||
| Setup::ManualToken => "manual_token".to_string(), | ||
| Setup::OAuth { scopes } => { | ||
| let mut scopes = scopes.clone(); | ||
| scopes.sort(); | ||
| format!("oauth:{}", scopes.join(",")) | ||
| } | ||
| Setup::Pairing => "pairing".to_string(), | ||
| Setup::Retired => "retired".to_string(), | ||
| } | ||
| } |
There was a problem hiding this comment.
To avoid unnecessary heap allocations for static variants (like ManualToken, Pairing, and Retired), we can return std::borrow::Cow<'static, str> instead of String. This keeps the code efficient and avoids allocating memory for static strings while still allowing dynamic formatting for the OAuth variant.
| fn stable_setup_token(setup: &ironclaw_host_api::RuntimeCredentialAccountSetup) -> String { | |
| use ironclaw_host_api::RuntimeCredentialAccountSetup as Setup; | |
| match setup { | |
| Setup::ManualToken => "manual_token".to_string(), | |
| Setup::OAuth { scopes } => { | |
| let mut scopes = scopes.clone(); | |
| scopes.sort(); | |
| format!("oauth:{}", scopes.join(",")) | |
| } | |
| Setup::Pairing => "pairing".to_string(), | |
| Setup::Retired => "retired".to_string(), | |
| } | |
| } | |
| fn stable_setup_token(setup: &ironclaw_host_api::RuntimeCredentialAccountSetup) -> std::borrow::Cow<'static, str> { | |
| use ironclaw_host_api::RuntimeCredentialAccountSetup as Setup; | |
| match setup { | |
| Setup::ManualToken => std::borrow::Cow::Borrowed("manual_token"), | |
| Setup::OAuth { scopes } => { | |
| let mut scopes = scopes.clone(); | |
| scopes.sort(); | |
| std::borrow::Cow::Owned(format!("oauth:{}", scopes.join(","))) | |
| } | |
| Setup::Pairing => std::borrow::Cow::Borrowed("pairing"), | |
| Setup::Retired => std::borrow::Cow::Borrowed("retired"), | |
| } | |
| } |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | f7fc2f698220 |
Head: f7fc2f6982203bb3d4b4736c93b9f1bf2bb5b9b7
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
The auth-gate fingerprint still collides for distinct OAuth setup scope vectors containing commas, so the stale gate-record failure remains possible.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Encode OAuth setup scopes unambiguously in the gate fingerprint
Location: crates/ironclaw_host_runtime/src/production.rs:2163
scopes.join(",") is not an injective representation. For example, OAuth setups with scopes = ["a,b"] and scopes = ["a", "b"] produce the same token, while provider_scopes and every other fingerprint field can be identical. Setup scopes have no validation here that forbids commas, so these distinct auth flows still derive the same gate key and can hit the GateRecordAlreadyExists path with a stale record. Use an unambiguous canonical encoding (for example, length-prefixed elements or canonical serialized data) and add this collision case to the regression test.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| Setup::OAuth { scopes } => { | ||
| let mut scopes = scopes.clone(); | ||
| scopes.sort(); | ||
| format!("oauth:{}", scopes.join(",")) |
There was a problem hiding this comment.
join(",") is ambiguous: scopes = ["a,b"] and scopes = ["a", "b"] yield the same setup token. Since setup scopes are not validated to reject commas, these distinct auth flows can still share a gate key and retain a stale GateRecord. Please use an unambiguous canonical encoding and add this regression case.
|
🚅 Deployed to the ironclaw-pr-6303 environment in ironclaw-ci-preview
|
…6303 IronLoop) `scopes.join(",")` is not injective: `["a,b"]` and `["a", "b"]` produce the same token, and setup/provider scopes are unvalidated for commas — so two distinct OAuth auth flows could still derive the same `auth-{sha256}` gate key and hit the `GateRecordAlreadyExists` stale-record path. Add `canonical_scope_list` — sort (scope sets are order-independent) then length-prefix each element (`<byte_len>:<scope>`) so distinct sets never share an encoding regardless of characters — and route BOTH the per-requirement `provider_scopes` and the OAuth `stable_setup_token` scopes through it. Regression: extended `auth_required_outcome_changes_gate_when_only_setup_changes` with the `["a,b"]` vs `["a", "b"]` collision case — verified it fails on the old `join(",")`. clippy + fmt clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the blocking finding ( MED — non-injective scope encoding. Added Regression: extended (FWIW I'd independently opened #6304 for the original setup-omission finding before spotting this PR — closed it in favor of yours, which is more complete.) @ironloopai review |
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | 7c63e055224c |
Head: 7c63e055224c0aad8ddf71d91035d453514ed070
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
The auth-gate fingerprint implementation is sound on inspection, but its new provider-scope collision path lacks a regression test.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Add a provider_scopes-only collision regression
Location: crates/ironclaw_host_runtime/src/production.rs:2699-2704
This collision case varies Setup::OAuth.scopes, while requirement_with keeps provider_scopes fixed at ["read"]. It therefore still passes if the provider_scopes call at the fingerprint were reverted to join(","), leaving that changed collision path unprotected. Add a case with fixed ManualToken setup and provider_scopes ["a,b"] versus ["a", "b"], asserting distinct gate IDs.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| // delimiter must not collide with two scopes that join to the same | ||
| // string — `["a,b"]` and `["a", "b"]` are DIFFERENT scope sets. Before | ||
| // the length-prefixed `canonical_scope_list`, both encoded to "a,b". | ||
| let one_comma_scope = gate_id(Setup::OAuth { |
There was a problem hiding this comment.
This only exercises the OAuth setup-scope encoding; provider_scopes stays fixed. Please add a ManualToken case varying only provider_scopes between ["a,b"] and ["a", "b"], so a regression of that separate fingerprint input is caught.
…ion (#6303 IronLoop) The existing collision case varied OAuth setup scopes while holding provider_scopes fixed, so it still passed if only the provider_scopes encoding were reverted to join(","). Add a fixed-ManualToken case with provider_scopes ["a,b"] vs ["a", "b"] asserting distinct gate ids — verified it fails when only the provider_scopes call is reverted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the follow-up ( (The |
…abbit-cleanup-6299
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 86.2% — 319556 / 370720 lines Per-crate breakdown (65 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)
|
…uest-side) collapse plan (#6306) §14 status log was stale — it listed the §5.3 five-channel flip as "in flight on integration/reborn-flip-base" and said "CapabilityOutcome is retained for Stage 2b to delete", but that work has landed on main: - Move the §5.3 flip stack from "In flight" to "Merged"; record #6293 (Stage 2b — CapabilityOutcome + all result mirrors DELETED), #6299 (the stack squash-landed on main, reconciled with #6279/#6277/#6292/#6296), and #6303 (auth-gate setup fingerprint fix + injective encoding). - Fix the Slice C.1 bullet: the Resolution/Blocked/Suspension/HostFailure channel enums are now merged too. - Add the remaining work under "Not started": the Slice C down-path (request-side) collapse — the 9 request mirrors still frozen in FROZEN_COLLAPSE_DTOS — with the concrete risk-ordered slice sequence (D1 dispatch→Authorized, D2 authorize(&Invocation), D3 loop membrane mints Invocation, D4 resume/auth-resume, D5 security-milestone seal inline, D6 ratchet-to-empty + measure). Docs-only; the frozen contract (§1–§13) is unchanged, only the mutable §14 log. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Summary
Follow-up to #6299 (capability-result collapse), addressing the review findings
that were left for a follow-up so #6299 could land. Two independent commits:
1.
fix(host-runtime): include credentialsetupin the auth-gate fingerprint (IronLoop, blocking)The deterministic auth-gate id (
stable_auth_gate_id) hashed the capability,required secret handles, and each credential requirement's
provider/requester/
provider_scopes— but not itssetup(
RuntimeCredentialAccountSetup, including OAuth setup scopes). Two authrequirements identical except for
setup(a ManualToken record vs a laterOAuth/Pairing record, or two OAuth setups with different setup scopes) derived
the same
for_auth_gatekey. At the loop-host persist seam the write-oncegate-record store then reports
GateRecordAlreadyExistsand — treating adeterministic-key collision as a benign byte-identical re-raise — keeps the
stale record; the runner reloads it and renders the wrong authentication
flow.
Fix: fold a canonical
setuptoken into the fingerprint (stable_setup_token,exhaustive over the enum, OAuth setup scopes sorted). Distinct setups now derive
distinct keys and never reach the collision branch with a stale record.
stable_auth_gate_idis the sole auth-gate fingerprint minter (verified), sothis closes it at the single source. Regression test
auth_required_outcome_changes_gate_when_only_setup_changesvaries onlysetupand fails before / passes after.2.
refactor(reborn): CodeRabbit test-only cleanupsassert_recoverable_failuretest helper (inlocal_dev/outbound_delivery.rs+local_dev/project_create.rs) into one#[cfg(test)] pub(crate)helper inlocal_dev.rs.approval_gate_ref(&Resolution) -> LoopGateRefhelper inironclaw_runner/tests/hooks_integration.rs, replacing the sameBlocked::Approval → origin() → LoopGateRefshape repeated 6×.RebornCapabilityBackend::gate_record_storeintests/integration/support/group.rs(the host-runtime arm always returnsSome).CodeRabbit's Cargo.toml feature-gate nit does not apply:
ironclaw_capabilitieshas no
[features]section and the consumedFilesystemReplayPayloadStoreis aproduction type, so the bare dev-dependency is correct.
Verification
cargo test -p ironclaw_host_runtime --all-features --lib— 347 passed(incl. the new auth-gate regression).
cargo test -p ironclaw_reborn_composition --lib local_dev— 236 passed.cargo test -p ironclaw_runner --test hooks_integration— 49 passed.cargo clippy -p ironclaw_host_runtime -p ironclaw_loop_host --all-targets --all-features -- -D warnings— clean;cargo fmt --all --check— clean.🤖 Generated with Claude Code