Fix Reborn credential delete and same-run reauth

[henrypark133]

Summary

Fixes Reborn credential delete and reliable re-auth behavior for runtime credentials.

• deletes the GitHub runtime PAT through the Reborn credential delete route and product-auth account state instead of legacy setup credential discovery
• marks rejected runtime credentials from 401 responses and recovers them through Reborn product-auth ownership
• returns same-run AuthRequired after a recovered 401 instead of waiting for the next tool use
• keeps OAuth refresh behavior intact: successful refresh continues, failed/no-op refresh opens re-auth, and 403 does not revoke or open auth
• keeps the same-run reauth handoff private to Reborn composition via RuntimeCredentialReauthBridge; host runtime does not own product-auth reauth state
• compares full RuntimeCredentialUnauthorized markers for ambiguity so conflicting recovery metadata does not pick the wrong account flow

User experience

Example: GitHub PAT expires or is revoked while an extension tool is running.

Before:

• tool call gets 401
• account may be revoked silently
• user only sees auth on a later retry / next tool use

After:

• tool call gets 401
• Reborn revokes or refreshes the exact staged credential account if it is still the same account revision
• the current run immediately returns an auth gate for the affected extension/account
• user can paste a replacement token in the same flow

OAuth providers keep their refresh path: if refresh produces a new configured token, the run continues; if refresh fails or leaves the rejected token unchanged, the same-run auth gate opens.

Validation

• cargo fmt --check
• cargo check -p ironclaw_host_runtime -p ironclaw_reborn_composition
• cargo test -p ironclaw_reborn_composition runtime_credential_ --lib
• cargo test -p ironclaw_host_runtime host_runtime_http_egress_skips_credential_unauthorized_marker_when_recovery_metadata_differs --lib
• cargo test -p ironclaw_host_runtime host_runtime_http_egress --lib --test reborn_e2e_gate --test runtime_http_egress_contract --test github_wasm_runtime_contract --test builtin_obligation_handler_contract --no-run
• cargo test -p ironclaw_host_api runtime_http_egress_response_round_trips_optional_credential_unauthorized --test host_api_contract
• cargo test -p ironclaw_auth refresh_contract --test auth_product_contract

Final thermo review loop: no Critical/High findings.

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Added a new account revocation action in product auth, letting authorized callers revoke credential accounts.
  • Improved runtime handling of credential-related HTTP 401 responses, with better recovery and retry behavior.

• Bug Fixes

  • Conditional refresh/revoke flows now avoid changing accounts when the underlying data has already changed.
  • Unauthorized credential details are now carried through responses more consistently, improving recovery from auth failures.

Walkthrough

Adds conditional credential mutation methods, carries credential-account metadata through staging and egress, adds typed unauthorized markers to HTTP egress responses, and wires 401 recovery plus reauth into composition. Also adds a product-auth accounts revoke route and updates affected fixtures and tests.

Changes

Credential Unauthorized Recovery & Conditional Account Mutation

Layer / File(s)	Summary
Conditional account mutation API crates/ironclaw_auth/src/credential.rs	Adds revoke_if_unchanged and refresh_if_unchanged to the credential service trait; extends provider-backed refresh handling to return None on expected updated_at mismatches.
Conditional revoke implementations crates/ironclaw_auth/src/fakes.rs, crates/ironclaw_auth/tests/auth_product_contract/refresh_contract.rs, crates/ironclaw_reborn_composition/src/product_auth_durable/accounts.rs	Implements conditional revoke in the in-memory and filesystem-backed credential services and adds contract coverage for stale markers and requester-extension checks.
Accounts revoke endpoint crates/ironclaw_reborn_composition/src/product_auth_serve/accounts.rs, crates/ironclaw_reborn_composition/src/product_auth_serve/mod.rs	Adds the revoke request type, handler, route wiring, ingress descriptor entry, and route test for the product-auth accounts revoke endpoint.
Unauthorized marker types and response field crates/ironclaw_host_api/src/http.rs, crates/ironclaw_host_api/src/ids.rs, crates/ironclaw_host_api/tests/host_api_contract.rs	Adds the runtime credential account identity and unauthorized marker types, extends the HTTP egress response with credential_unauthorized, and adds JSON round-trip coverage.
Resolver populates unauthorized policy crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rs, crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests.rs	Updates runtime credential resolution to derive unauthorized policy, requester extension, and auth requirements, and adds tests for the returned identity data.
Credential-account metadata through staging crates/ironclaw_host_runtime/src/obligations.rs, crates/ironclaw_host_runtime/src/services.rs, crates/ironclaw_host_runtime/src/services/tests.rs, crates/ironclaw_host_runtime/src/wasm_credentials.rs, crates/ironclaw_reborn_composition/src/slack_egress.rs	Adds credential_account to resolved access secrets and staged secret material, updates the injection store to preserve that metadata, and threads it through runtime staging and restaging paths.
Injection result and 401 marker attachment crates/ironclaw_host_runtime/src/egress/credential.rs, crates/ironclaw_host_runtime/src/egress/mod.rs, crates/ironclaw_host_runtime/src/egress/pipeline.rs	Introduces CredentialInjectionResult, makes credential injection operate on staged material with account metadata, suppresses ambiguous markers, and attaches the marker only on 401 responses.
Host-port staging and marker propagation crates/ironclaw_host_runtime/src/egress/host_port.rs	Adds credential-account-aware staging in the host runtime egress port and updates the host-port tests for 401 marker propagation.
Reauth bridge and host runtime wrapper crates/ironclaw_reborn_composition/src/runtime_credential_reauth.rs	Introduces the recovered-auth bridge, the host runtime wrapper that replaces outcomes with AuthRequired, and deterministic gate-id derivation from sorted credential requirements.
Unauthorized recovery egress crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized.rs, crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized/tests.rs	Wraps runtime HTTP egress to consume credential_unauthorized on 401 responses, conditionally revoke or refresh accounts, and record recovered auth requirements.
Composition wiring crates/ironclaw_reborn_composition/src/factory.rs, crates/ironclaw_reborn_composition/src/lib.rs	Adds the unauthorized-recovery composition helper and wires the reauth bridge, recovery egress, and wrapped host runtime into local-dev and production factory paths.
Fixture shape updates Multiple test files across crates	Updates remaining test doubles, harnesses, and supporting edits to populate the new credential_unauthorized and credential_account fields with None where applicable.

Sequence Diagram(s)

sequenceDiagram
  participant Guest as Runtime guest
  participant Pipeline as Egress pipeline
  participant Inject as apply_credential_injections
  participant Recover as RuntimeCredentialUnauthorizedRecoveryEgress
  participant Accounts as CredentialAccountService
  participant Bridge as RuntimeCredentialReauthBridge
  participant Reauth as RuntimeCredentialReauthHostRuntime

  Guest->>Pipeline: HTTP egress request
  Pipeline->>Inject: apply_credential_injections(...)
  Inject-->>Pipeline: CredentialInjectionResult
  Pipeline->>Recover: execute(request)
  Recover->>Accounts: revoke_if_unchanged(...) or refresh_if_unchanged(...)
  Accounts-->>Recover: Some(...) or None
  Recover->>Bridge: record_recovered_auth_required(...)
  Pipeline-->>Guest: RuntimeHttpEgressResponse
  Reauth->>Reauth: invoke/spawn/resume → AuthRequired when bridge has records

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• nearai/ironclaw#5087: Modifies the same refresh_account_from_snapshot terminal-refresh branches in crates/ironclaw_auth/src/credential.rs that this PR extends with expected_updated_at checks.

Suggested reviewers

• serrrfirat

Poem

Ok(None) at the gate,
and 401 wears a marker bright.
The bridge remembers what was recovered,
then lifts AuthRequired into sight.
Staged secrets keep their ledger true,
while revoke and refresh wait for the right hue.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description covers summary and validation, but it omits most required template sections and is not mostly complete.	Add the missing template sections: Change Type, Linked Issue, Security Impact, Trust-Boundary Checklist, Database Impact, Blast Radius, Rollback Plan, Review Follow-Through, and Review track.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the PR’s main change, though it does not use the preferred Conventional Commits format.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands.}

[gemini-code-assist]

Code Review

This pull request introduces a robust mechanism to handle unauthorized (401) responses for injected credentials in host-staged runtimes. It adds conditional revocation and refresh capabilities (revoke_if_unchanged and refresh_if_unchanged) to the CredentialAccountService, and integrates an unauthorized recovery pipeline via RuntimeCredentialUnauthorizedRecoveryEgress and a re-authentication bridge. Feedback focuses on addressing potential race conditions in the default non-atomic implementations of the new service methods, and replacing warn! and info! logging with debug! in REPL/TUI-reachable code to prevent interface corruption.

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.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3a3ed3d17c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

crates/ironclaw_auth/src/credential.rs (1)

967-984: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Return None when terminal refresh races a newer account.

After provider.refresh_token(...).await, another actor may have reauthorized or updated the account. In the InvalidGrant / RefreshFailed arms, report_terminal_refresh_status() returns a report for the changed current account, and Lines 967/976 wrap that as Some, so recovery treats a stale marker as handled instead of skipped. Thread expected_updated_at into this terminal-status path and return Ok(None) on mismatch.

Suggested shape

-                Err(AuthProductError::InvalidGrant) => self
-                    .report_terminal_refresh_status(
+                Err(AuthProductError::InvalidGrant) => self
+                    .report_terminal_refresh_status_if_unchanged(
                         &lookup_request,
                         &account,
+                        expected_updated_at,
                         request.requester_extension.as_ref(),
                         CredentialAccountStatus::Revoked,
                     )
-                    .await
-                    .map(Some),
+                    .await,

Apply the same pattern to the RefreshFailed | TokenExchangeFailed arm.

Based on PR objectives and the recovery wrapper contract, stale expected_updated_at mismatches must surface as Ok(None).

🤖 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 `@crates/ironclaw_auth/src/credential.rs` around lines 967 - 984, The
InvalidGrant and RefreshFailed error arms currently always wrap the
report_terminal_refresh_status result as Some, but they should check for
expected_updated_at mismatches and return Ok(None) when the account has been
updated by another actor since the refresh attempt. Thread the
expected_updated_at value into the report_terminal_refresh_status calls in both
error arms (line 967 and line 976), then add logic to check if the returned
report indicates a mismatch with expected_updated_at; if mismatched, return
Ok(None) instead of Ok(Some(...)) to properly surface stale markers as skipped
rather than handled.

crates/ironclaw_host_runtime/src/egress/host_port.rs (1)

155-221: 📐 Maintainability & Code Quality | 🟠 Major

Remove dead marker-computation code from stage_credentials; the inner pipeline already computes it.

Lines 178–220 duplicate the exact ambiguity-suppression + marker-selection logic that apply_credential_injections performs in egress/credential.rs via record_unauthorized_identity. However, the staged_unauthorized_identity computed here is never passed to the request. The inner runtime_http_egress.execute() (which is HostHttpEgressService, running pipeline::execute) independently reads each credential's staged credential_account and recomputes the marker, returning it in CredentialInjectionResult. By the time the outer .map(...) at lines 159–165 calls attach_credential_unauthorized_on_401, the pipeline has already set response.credential_unauthorized, so the guard is_none() prevents the host_port's copy from ever being applied — making those 43 lines dead code.

The test passes only because RecordingRuntimeHttpEgress is a stub that skips the pipeline entirely; thus the test exercises logic that never fires in production (violates "test through the caller").

Fix: Delete the stage_credentials marker logic (lines 178–220); it adds no value and masks the real source of truth.

🤖 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 `@crates/ironclaw_host_runtime/src/egress/host_port.rs` around lines 155 - 221,
In the `stage_credentials` method, remove all the marker computation logic
including the declarations of `unauthorized_identity` and
`ambiguous_unauthorized_identity`, the conditional block that checks for
`credential_account` and calls `marker_on_unauthorized()`, and the final return
statement that evaluates the ambiguity condition. Instead, simply return
`Ok(None)` at the end of the method after staging the credentials and pushing
all credential injections, since the pipeline's `apply_credential_injections`
function already handles marker computation independently.

🤖 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 `@crates/ironclaw_auth/src/credential.rs`:
- Around line 510-549: The revoke_if_unchanged and refresh_if_unchanged methods
perform a read-then-write operation that is not atomic. They check if
account.updated_at matches expected_updated_at, then call self.update_status or
self.refresh_account separately, creating a race condition where the account
could be modified between the check and mutation. To fix this, refactor the
mutation operations (self.update_status and self.refresh_account) to accept the
expected_updated_at timestamp as a parameter and perform the revision check
atomically within the mutation call itself, using a compare-and-swap or
transactional approach so that the timestamp validation and state mutation
happen as a single atomic operation.

In `@crates/ironclaw_host_api/src/http.rs`:
- Around line 117-128: The account_id field is currently defined as a raw String
type, but according to coding guidelines identifiers should use newtypes for
type safety. Create a new validated newtype called RuntimeCredentialAccountId
(or reuse an existing account-id type from a crate dependency if the dependency
direction allows it), and then replace the account_id String field in the struct
definition shown in the diff with this new typed identifier. Also apply the same
change to the account_id fields in the other structs mentioned in lines 354-383
to ensure consistency across all identity, unauthorized marker, and key structs.

In `@crates/ironclaw_host_runtime/src/egress/credential.rs`:
- Around line 287-292: The early return when marker_on_unauthorized() returns
None (due to missing account_updated_at or auth_requirement) causes incomplete
credential-account metadata to be treated permissively, allowing markerable
credentials to win when paired with unmarkerable ones whose 401s cannot be
safely attributed. Remove the early return pattern that bypasses marker
suppression when marker_on_unauthorized() is None, and instead treat incomplete
account metadata as an ambiguous scenario where the marker should be suppressed,
ensuring the function fails closed by not proceeding with the marker when
account metadata is incomplete.

In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 1228-1234: The RuntimeCredentialReauthBridge wrapper is being
applied to services via attach_runtime_credential_unauthorized_recovery, but
subsequent code paths are bypassing this wrapper by using
product_auth_runtime_ports.runtime_http_egress() directly instead of
services.runtime_http_egress(). Locate all places where
product_auth_runtime_ports.runtime_http_egress() is assigned or passed after the
attach_runtime_credential_unauthorized_recovery call, including assignments to
local_runtime.runtime_http_egress and ProductAuthRuntimeGsuiteCredentialStager
initialization at lines 1305, 1327, 3857 and elsewhere. Verify whether these
code paths intentionally require unwrapped egress for OAuth provider
interactions, and if not, replace
product_auth_runtime_ports.runtime_http_egress() with
services.runtime_http_egress() to ensure the credential recovery wrapper is
applied consistently.

In `@crates/ironclaw_reborn_composition/src/runtime_credential_reauth.rs`:
- Around line 123-126: The invoke_capability method (and similarly
spawn_capability, resume_capability, auth_resume_capability, and
resume_spawn_capability methods) currently calls
self.inner.invoke_capability(request).await? which exits early on error before
apply_reauth can be called. This leaves stale bridge records in memory. Modify
the error handling to drain the matching bridge record using the scope and
capability_id before returning the error, either by wrapping the invocation in a
match statement or extracting a helper function that handles both the success
path (calling apply_reauth) and error path (draining the record). Apply this
pattern consistently across all four methods mentioned in the review comment.
- Around line 45-48: The RuntimeCredentialReauthRecord struct needs to store the
full ResourceScope to satisfy the scoped-state invariant. Add a field to the
RuntimeCredentialReauthRecord struct to hold the complete ResourceScope, then
update the record creation around line 45 to include the full scope from the
scope object (alongside invocation_id and capability_id), and finally update the
matching/lookup logic around lines 66-68 to compare the full ResourceScope in
addition to invocation_id and capability_id to prevent reused invocation IDs
from consuming another owner's recovered auth requirements.

In `@crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized.rs`:
- Around line 51-52: Add a validation check to ensure that the scope derived
from unauthorized.scope matches the request_scope before proceeding with
credential mutations. After creating the scope variable using
AuthProductScope::credential_owner with unauthorized.scope, add a guard
condition that compares this scope against request_scope and returns an error if
they do not match. Apply the same scope validation pattern to the code block at
lines 64-72 that also performs credential operations. This prevents mis-scoped
markers from mutating credentials across different authorization contexts.
- Around line 112-118: The Err branches in the recovery function (around line
112-118 and lines 161-167) are currently logging warnings and returning false,
which silently swallows authentication and state infrastructure failures.
Instead of warn-and-continue, these error cases should propagate the error to
the caller. Modify the Err(error) branches where tracing::warn! is called to
return an error result instead of returning false, allowing the recovery
operation to fail loudly in execute() as intended. Keep the Ok(None) case
unchanged as a valid no-op for stale accounts.
- Around line 40-53: The code currently treats the presence of
credential_unauthorized field as sufficient authority to revoke or refresh
credentials, but it should also verify the HTTP status code is 401 before
proceeding with credential mutation. Add an additional guard condition to check
that response.status equals 401 before allowing the credential_unauthorized
processing logic to continue. This ensures that only genuine 401 Unauthorized
responses trigger credential mutation, preventing malformed or buggy responses
with other status codes (like 403 or 200) from incorrectly revoking credentials.
Place this status code check at the beginning of the function, ideally before or
immediately after the existing credential_unauthorized check.
- Around line 177-178: The map_err closure on the AuthProviderId::new call is
ignoring the actual parse error by using an underscore binding, which drops
valuable debugging information. Replace the ignored error binding with a named
binding (e.g., err) and either include that error detail in the
AuthProductError::MalformedConfig error type if it supports carrying causes, or
log the specific error at debug! level before mapping to the generic
MalformedConfig error. This ensures the underlying cause of the AuthProviderId
parse failure is preserved for troubleshooting.

---

Outside diff comments:
In `@crates/ironclaw_auth/src/credential.rs`:
- Around line 967-984: The InvalidGrant and RefreshFailed error arms currently
always wrap the report_terminal_refresh_status result as Some, but they should
check for expected_updated_at mismatches and return Ok(None) when the account
has been updated by another actor since the refresh attempt. Thread the
expected_updated_at value into the report_terminal_refresh_status calls in both
error arms (line 967 and line 976), then add logic to check if the returned
report indicates a mismatch with expected_updated_at; if mismatched, return
Ok(None) instead of Ok(Some(...)) to properly surface stale markers as skipped
rather than handled.

In `@crates/ironclaw_host_runtime/src/egress/host_port.rs`:
- Around line 155-221: In the `stage_credentials` method, remove all the marker
computation logic including the declarations of `unauthorized_identity` and
`ambiguous_unauthorized_identity`, the conditional block that checks for
`credential_account` and calls `marker_on_unauthorized()`, and the final return
statement that evaluates the ambiguity condition. Instead, simply return
`Ok(None)` at the end of the method after staging the credentials and pushing
all credential injections, since the pipeline's `apply_credential_injections`
function already handles marker computation independently.

🪄 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: cfa2ebe2-5b96-4baa-af8c-ee20eeb9bda7

📥 Commits

Reviewing files that changed from the base of the PR and between e754655 and 3a3ed3d.

📒 Files selected for processing (47)

• crates/ironclaw_auth/src/credential.rs
• crates/ironclaw_auth/src/fakes.rs
• crates/ironclaw_auth/tests/auth_product_contract/refresh_contract.rs
• crates/ironclaw_first_party_extensions/src/gsuite/handlers.rs
• crates/ironclaw_first_party_extensions/src/web_access.rs
• crates/ironclaw_first_party_extensions/tests/support/mod.rs
• crates/ironclaw_host_api/src/http.rs
• crates/ironclaw_host_api/tests/host_api_contract.rs
• crates/ironclaw_host_runtime/src/egress/credential.rs
• crates/ironclaw_host_runtime/src/egress/host_port.rs
• crates/ironclaw_host_runtime/src/egress/mod.rs
• crates/ironclaw_host_runtime/src/egress/pipeline.rs
• crates/ironclaw_host_runtime/src/obligations.rs
• crates/ironclaw_host_runtime/src/services/tests.rs
• crates/ironclaw_host_runtime/src/wasm_credentials.rs
• crates/ironclaw_host_runtime/tests/builtin_obligation_handler_contract.rs
• crates/ironclaw_host_runtime/tests/first_party_builtin_tools.rs
• crates/ironclaw_host_runtime/tests/github_wasm_runtime_contract.rs
• crates/ironclaw_host_runtime/tests/host_runtime_services_contract.rs
• crates/ironclaw_host_runtime/tests/runtime_http_egress_contract.rs
• crates/ironclaw_mcp/tests/mcp_adapter_contract.rs
• crates/ironclaw_reborn_composition/src/auth_dcr_tests.rs
• crates/ironclaw_reborn_composition/src/extension_lifecycle.rs
• crates/ironclaw_reborn_composition/src/extension_lifecycle/hosted_mcp_test_support.rs
• crates/ironclaw_reborn_composition/src/factory.rs
• crates/ironclaw_reborn_composition/src/factory/auth_tests.rs
• crates/ironclaw_reborn_composition/src/lib.rs
• crates/ironclaw_reborn_composition/src/oauth_dcr.rs
• crates/ironclaw_reborn_composition/src/oauth_provider_client.rs
• crates/ironclaw_reborn_composition/src/oauth_provider_client/tests.rs
• crates/ironclaw_reborn_composition/src/product_auth_durable/accounts.rs
• crates/ironclaw_reborn_composition/src/product_auth_providers.rs
• crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rs
• crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests.rs
• crates/ironclaw_reborn_composition/src/product_auth_serve/accounts.rs
• crates/ironclaw_reborn_composition/src/product_auth_serve/mod.rs
• crates/ironclaw_reborn_composition/src/product_auth_serve/oauth_start_tests.rs
• crates/ironclaw_reborn_composition/src/projection/tests/turn_stream_auth.rs
• crates/ironclaw_reborn_composition/src/runtime_credential_reauth.rs
• crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized.rs
• crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized/tests.rs
• crates/ironclaw_reborn_composition/src/slack_egress.rs
• crates/ironclaw_reborn_composition/tests/gsuite.rs
• crates/ironclaw_scripts/tests/script_http_adapter_contract.rs
• crates/ironclaw_wasm/tests/wasm_dispatch_integration.rs
• crates/ironclaw_wasm/tests/wasm_http_adapter_contract.rs
• tests/support/reborn/harness.rs

[railway-app]

🚅 Deployed to the ironclaw-pr-5174 environment in ironclaw-ci-preview

Service	Status	Web	Updated (UTC)
ironclaw	✅ Success (View Logs)	Web	Jun 24, 2026 at 5:06 am

[henrypark133]

Code Review (multi-agent)

Intent: Fix Reborn runtime credential deletion and same-run reauth so 401 recovery immediately gates the current run without breaking OAuth refresh behavior.

Stats: 3 findings (from 8 raw reviewer findings; 5 after local validation; 3 after deduping against current live review threads) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.

Dedup note: current live threads already cover the non-atomic conditional mutation concern, the anonymous map_err(|_| ...) provider parse mapping, and incomplete-marker behavior, so this review does not repost those.

Tests

1. Medium Conditional revoke lacks requester-extension coverage (crates/ironclaw_auth/src/credential.rs:510-529, confidence 78) — anchor: crates/ironclaw_auth/src/credential.rs:510
   The new revoke_if_unchanged path is only exercised for the no-requester case. There is no test proving that a carried requester_extension is included in the lookup and still revokes an extension-owned account when the timestamp matches.

Maintainability

1. Medium Collapse the split staged-secret metadata API (crates/ironclaw_host_runtime/src/obligations.rs:100-210, confidence 84) — anchor: crates/ironclaw_host_runtime/src/obligations.rs:130
   This turns one secret-injection store into two parallel API families (insert/take/clone_material and their *_with_metadata variants) plus a wrapper type that just carries material and credential_account. That makes every caller choose between near-identical paths and keeps the metadata flow duplicated across the store, the stager, and the egress code.

2. Low Split the stale-aware refresh path from the normal refresh path (crates/ironclaw_auth/src/credential.rs:875-987, confidence 68) — anchor: crates/ironclaw_auth/src/credential.rs:875
refresh_account_from_snapshot now uses an Option<Timestamp> as a hidden mode switch, and the stale check is repeated before the lock, after the reread, and again in the provider-refresh branch. That makes one helper carry two contracts at once and forces readers to keep the None/Some meaning in their head while following the refresh flow.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)

crates/ironclaw_auth/src/credential.rs (1)

815-819: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update the stale-write contract comment.

The helper now has two behaviors when another writer wins: normal refresh reports the current state, but expected-revision refresh returns Ok(None). The comment still describes only the old plain-report path.

As per coding guidelines, “Comments that promise guarantees across layers must either be enforced by code/tests or softened to describe intent.”

Suggested comment update

-    /// first and bails to a plain report if another writer changed it under us.
+    /// first; normal refresh reports the current state if another writer changed
+    /// it, while expected-revision refresh returns `Ok(None)`.

🤖 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 `@crates/ironclaw_auth/src/credential.rs` around lines 815 - 819, The comment
for the helper function at lines 815-819 in credential.rs describes outdated
behavior for handling stale writes. Update the comment to reflect the current
two-path behavior: clarify that when another writer changes the account under
us, normal refresh reports the current state while expected-revision refresh
returns Ok(None). Remove references to the old plain-report path behavior and
ensure the comment accurately describes both code paths that handle the
stale-write condition.

Source: Coding guidelines

crates/ironclaw_host_runtime/src/egress/host_port.rs (2)

61-69: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Preserve the secret-staging failure cause.

map_err(|_| RuntimeSecretStageError::Backend) drops the insert failure at the secret/egress boundary. Bind the error and carry it through a contextual variant, or log sanitized debug context before mapping.

As per coding guidelines, “Do not use .map_err(|_| OtherError) … Carry the cause instead,” and as per path instructions, “Fail loud.”

🤖 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 `@crates/ironclaw_host_runtime/src/egress/host_port.rs` around lines 61 - 69,
The map_err call in the insert_with_credential_account method is dropping the
error information by using the ignore pattern |_|, which prevents debugging of
secret staging failures. Modify the map_err to capture the actual error (bind it
to a variable instead of using underscore) and either pass it through a
contextual error variant that preserves the cause or log the sanitized error
details before mapping to RuntimeSecretStageError::Backend. This ensures error
context is preserved for failure diagnosis while maintaining security by
avoiding sensitive data exposure in logs.

Sources: Coding guidelines, Path instructions

---

166-174: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Reject cross-scope credential-account metadata before staging.

Line 166 accepts credential_account without proving it belongs to request.scope; the new 401 fixtures create marker/account scopes independently from host_request(), proving this path accepts mismatched metadata. That breaks same-run recovery and can bind staged secret material to the wrong auth recovery record. Validate the scope before insert_with_credential_account, and update the 401 tests to derive marker/account scope from request.request.scope.

As per coding guidelines, “Preserve tenant/user/agent/project/mission/thread scope on authority, state, memory, process, network, outbound, resource, and event records.”

Suggested validation

         for credential in credentials {
+            if let Some(account) = credential.credential_account.as_ref() {
+                if &account.scope != &request.scope {
+                    return Err(RuntimeHttpEgressError::Credential {
+                        reason: "host credential account metadata did not match request scope"
+                            .to_string(),
+                    });
+                }
+            }
             let credential_account = credential.credential_account.clone();
             self.secret_stager
                 .stage_secret_material_once_with_account(

Also applies to: 449-486, 535-570

🤖 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 `@crates/ironclaw_host_runtime/src/egress/host_port.rs` around lines 166 - 174,
The credential_account parameter is being passed to
stage_secret_material_once_with_account without validating that it belongs to
the same scope as request.scope, which can cause secret material to be bound to
the wrong auth recovery record. Add a validation check before the call to
stage_secret_material_once_with_account that verifies the credential_account
scope matches request.scope, and reject it with appropriate error handling if
they do not match. Apply this same validation pattern to the other similar
credential staging calls in the file at the locations noted in the comment.

Source: Coding guidelines

crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized/tests.rs (1)

717-744: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Make the no-op refresh fake validate the full request.

This fake ignores request.requester_extension and request.provider, so it can pass even if recovery calls refresh for the wrong extension/provider. Mirror production validation before returning the no-op report.

Validate requester and provider in the fake

-        let Some(account) = self
-            .inner
-            .get_account(CredentialAccountLookupRequest::new(
-                request.scope.clone(),
-                request.account_id,
-            ))
-            .await?
+        let mut lookup =
+            CredentialAccountLookupRequest::new(request.scope.clone(), request.account_id);
+        if let Some(requester_extension) = request.requester_extension.clone() {
+            lookup = lookup.for_extension(requester_extension);
+        }
+        let Some(account) = self.inner.get_account(lookup).await?
         else {
             return Ok(None);
         };
+        if account.provider != request.provider {
+            return Err(AuthProductError::CrossScopeDenied);
+        }
+        if account.status != CredentialAccountStatus::Configured {
+            return Err(AuthProductError::CredentialMissing);
+        }
         if account.updated_at != expected_updated_at {
             return Ok(None);
         }

As per path instructions, “When mocking a multi-arg runtime API, the mock must capture every argument the production caller passes.”

🤖 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
`@crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized/tests.rs`
around lines 717 - 744, The fake refresh_if_unchanged method currently validates
the account.updated_at against expected_updated_at but does not validate the
request.requester_extension and request.provider fields that the production
implementation would validate. Add validation checks for
request.requester_extension and request.provider before returning the
Ok(Some(CredentialRefreshReport)) to ensure the fake properly validates the full
request and aligns with the production behavior, preventing invalid recovery
calls from passing through undetected.

Source: Path instructions

crates/ironclaw_host_runtime/src/egress/credential.rs (1)

282-289: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Treat missing account metadata as ambiguous too.

Line 287 returns without suppressing a marker when another injected credential has no credential_account. A 401 from that request is still unattributable, so the account-backed credential can be revoked/refreshed incorrectly. Fail closed the same way as incomplete metadata.

Fail closed for non-account-backed injected credentials

     let Some(account) = account else {
+        *ambiguous_unauthorized_identity = true;
         return;
     };

As per coding guidelines, “Fail closed for auth, approvals, trust, filesystem containment, network policy, secret leases, runtime selection, and adapter identity.”

🤖 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 `@crates/ironclaw_host_runtime/src/egress/credential.rs` around lines 282 -
289, The record_unauthorized_identity function currently returns early without
setting any marker when the account is None, but missing credential_account
metadata makes the unauthorized identity unattributable and requires following
the "fail closed" principle for auth decisions. Modify the early return logic
(the let Some(account) = account else block) to set
ambiguous_unauthorized_identity to true before returning, ensuring that
credentials without account metadata are treated the same way as those with
incomplete metadata.

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.

Inline comments:
In `@crates/ironclaw_host_api/tests/host_api_contract.rs`:
- Around line 1150-1153: In the round-trip test for
RuntimeCredentialUnauthorized, change the `account_surface` field from the serde
default variant `RuntimeCredentialAccountSurface::Api` to a non-default variant
so that deserialization regressions would be caught. Then add an assertion after
deserialization to verify that the `account_surface` field in the deserialized
object matches the non-default variant that was set. Apply the same change to
both occurrences of this pattern in the test (around lines 1150-1153 and
1172-1190).

In `@crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized.rs`:
- Around line 51-57: The uuid::Uuid::parse_str call in the unauthorized marker
validation silently handles parse failures by returning Ok(()) instead of
failing loudly, which can hide contract breakage. Replace the silent downgrade
pattern (where parsing unauthorized.account_id fails and returns Ok(())) with
explicit error handling that propagates or clearly signals the failure, and
apply the same fix to the similar pattern around line 136 that currently returns
Ok(false) on parse failure.

---

Outside diff comments:
In `@crates/ironclaw_auth/src/credential.rs`:
- Around line 815-819: The comment for the helper function at lines 815-819 in
credential.rs describes outdated behavior for handling stale writes. Update the
comment to reflect the current two-path behavior: clarify that when another
writer changes the account under us, normal refresh reports the current state
while expected-revision refresh returns Ok(None). Remove references to the old
plain-report path behavior and ensure the comment accurately describes both code
paths that handle the stale-write condition.

In `@crates/ironclaw_host_runtime/src/egress/credential.rs`:
- Around line 282-289: The record_unauthorized_identity function currently
returns early without setting any marker when the account is None, but missing
credential_account metadata makes the unauthorized identity unattributable and
requires following the "fail closed" principle for auth decisions. Modify the
early return logic (the let Some(account) = account else block) to set
ambiguous_unauthorized_identity to true before returning, ensuring that
credentials without account metadata are treated the same way as those with
incomplete metadata.

In `@crates/ironclaw_host_runtime/src/egress/host_port.rs`:
- Around line 61-69: The map_err call in the insert_with_credential_account
method is dropping the error information by using the ignore pattern |_|, which
prevents debugging of secret staging failures. Modify the map_err to capture the
actual error (bind it to a variable instead of using underscore) and either pass
it through a contextual error variant that preserves the cause or log the
sanitized error details before mapping to RuntimeSecretStageError::Backend. This
ensures error context is preserved for failure diagnosis while maintaining
security by avoiding sensitive data exposure in logs.
- Around line 166-174: The credential_account parameter is being passed to
stage_secret_material_once_with_account without validating that it belongs to
the same scope as request.scope, which can cause secret material to be bound to
the wrong auth recovery record. Add a validation check before the call to
stage_secret_material_once_with_account that verifies the credential_account
scope matches request.scope, and reject it with appropriate error handling if
they do not match. Apply this same validation pattern to the other similar
credential staging calls in the file at the locations noted in the comment.

In
`@crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized/tests.rs`:
- Around line 717-744: The fake refresh_if_unchanged method currently validates
the account.updated_at against expected_updated_at but does not validate the
request.requester_extension and request.provider fields that the production
implementation would validate. Add validation checks for
request.requester_extension and request.provider before returning the
Ok(Some(CredentialRefreshReport)) to ensure the fake properly validates the full
request and aligns with the production behavior, preventing invalid recovery
calls from passing through undetected.

🪄 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: 8f122753-d5fc-455b-94a0-694e019cbc81

📥 Commits

Reviewing files that changed from the base of the PR and between 3a3ed3d and 13b1268.

📒 Files selected for processing (14)

• crates/ironclaw_auth/src/credential.rs
• crates/ironclaw_auth/src/fakes.rs
• crates/ironclaw_auth/tests/auth_product_contract/refresh_contract.rs
• crates/ironclaw_host_api/src/http.rs
• crates/ironclaw_host_api/tests/host_api_contract.rs
• crates/ironclaw_host_runtime/src/egress/credential.rs
• crates/ironclaw_host_runtime/src/egress/host_port.rs
• crates/ironclaw_reborn_composition/src/auth.rs
• crates/ironclaw_reborn_composition/src/factory.rs
• crates/ironclaw_reborn_composition/src/product_auth_durable/accounts.rs
• crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rs
• crates/ironclaw_reborn_composition/src/runtime_credential_reauth.rs
• crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized.rs
• crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized/tests.rs

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/ironclaw_host_runtime/src/egress/host_port.rs (1)

132-156: 🎯 Functional Correctness | 🟡 Minor

Cover the staged-account 401 path, not just marker pass-through. The host-port tests only use RecordingRuntimeHttpEgress::responding_with_marker(...), so they never prove credential_account staging feeds RuntimeCredentialAccountIdentity::marker_on_unauthorized() into the 401 response. Add a caller-level test that drives the real egress pipeline, or soften the line 132 comment.

🤖 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 `@crates/ironclaw_host_runtime/src/egress/host_port.rs` around lines 132 - 156,
The host-port coverage only verifies marker pass-through and does not exercise
the staged-account 401 path, so it never proves that staged credential_account
data is converted into
RuntimeCredentialAccountIdentity::marker_on_unauthorized() during unauthorized
responses. Update the tests around host_port::stage_credentials and the
RuntimeHttpEgress execution path to drive the real egress pipeline with staged
credentials, or adjust the comment in the host-port wrapper to match the
behavior actually covered. Use the existing
RecordingRuntimeHttpEgress::responding_with_marker setup as a reference, but add
a caller-level test that asserts the 401 response reflects the staged account
identity rather than only the marker.

Sources: Coding guidelines, Path instructions

🤖 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 `@crates/ironclaw_host_runtime/src/egress/host_port.rs`:
- Around line 132-156: The host-port coverage only verifies marker pass-through
and does not exercise the staged-account 401 path, so it never proves that
staged credential_account data is converted into
RuntimeCredentialAccountIdentity::marker_on_unauthorized() during unauthorized
responses. Update the tests around host_port::stage_credentials and the
RuntimeHttpEgress execution path to drive the real egress pipeline with staged
credentials, or adjust the comment in the host-port wrapper to match the
behavior actually covered. Use the existing
RecordingRuntimeHttpEgress::responding_with_marker setup as a reference, but add
a caller-level test that asserts the 401 response reflects the staged account
identity rather than only the marker.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 26afc585-686f-483a-a360-3f75c52ba7cf

📥 Commits

Reviewing files that changed from the base of the PR and between 13b1268 and 87622d6.

📒 Files selected for processing (12)

• crates/ironclaw_host_api/src/http.rs
• crates/ironclaw_host_api/src/ids.rs
• crates/ironclaw_host_api/tests/host_api_contract.rs
• crates/ironclaw_host_runtime/src/egress/credential.rs
• crates/ironclaw_host_runtime/src/egress/host_port.rs
• crates/ironclaw_host_runtime/src/obligations.rs
• crates/ironclaw_host_runtime/src/services.rs
• crates/ironclaw_host_runtime/src/services/tests.rs
• crates/ironclaw_host_runtime/src/wasm_credentials.rs
• crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rs
• crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized.rs
• crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized/tests.rs

[henrypark133]

Code Review (multi-agent)

Intent: Fix Reborn credential deletion and same-run reauthentication so revoked or refreshed runtime credentials gate the current run immediately.

Stats: 3 findings (from 6 raw, 3 after validation/dedup) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0

No High/Critical findings. This is a non-blocking review; the two Medium items are worth fixing before merge if you want the contract/test story tight, and the Low item is cleanup.

conventions

1. Medium Public host-api contract changed without matching docs update (crates/ironclaw_host_api/src/http.rs:113-185, confidence 72) — anchor: AGENTS.md:78
   This adds public host-api contract types for runtime credential identity and unauthorized recovery markers, but the branch does not update the matching Reborn contract docs. The repo rule requires behavior changes to update relevant docs/specs in the same branch, and docs/reborn/contracts/AGENTS.md lists host-api.md as the neutral host API vocabulary contract.

tests

1. Medium Refresh recovery needs a stale-marker skip test (crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized.rs:120-141, confidence 76) — anchor: crates/ironclaw_reborn_composition/src/runtime_credential_unauthorized.rs:120
   The new refresh recovery branch handles fresh markers and unchanged-refresh reauth, but the current caller-level stale-marker test only covers RevokeAccount. A stale RefreshAccount marker should be proven to skip same-run AuthRequired when the account has changed before recovery runs.

maintainability

1. Low Delete the unused unauthorized key wrapper (crates/ironclaw_host_api/src/http.rs:385-405, confidence 100) — anchor: crates/ironclaw_host_api/src/http.rs:385
   RuntimeCredentialUnauthorizedAccountKey and account_key() have no call sites in the tree. They export a second subset shape for the marker without removing complexity elsewhere, which makes the new public API larger than the PR currently needs.

[henrypark133]

Medium — Public host-api contract changed without matching docs update.

This adds public host-api contract types for runtime credential identity and unauthorized recovery markers, but the branch does not update the matching Reborn contract docs. The repo rule requires behavior changes to update relevant docs/specs in the same branch, and docs/reborn/contracts/AGENTS.md lists host-api.md as the neutral host API vocabulary contract.

Fix: Update docs/reborn/contracts/host-api.md and docs/reborn/contracts/host-runtime.md to describe the staged credential unauthorized marker and same-run recovery handoff.

[henrypark133]

Medium — Refresh recovery needs a stale-marker skip test.

The new refresh recovery branch handles fresh markers and unchanged-refresh reauth, but the current caller-level stale-marker test only covers RevokeAccount. A stale RefreshAccount marker should be proven to skip same-run AuthRequired when the account has changed before recovery runs.

Fix: Add runtime_credential_unauthorized_recovery_skips_stale_refresh_marker covering RefreshAccount when the account was updated after staging.

[henrypark133]

Low — Delete the unused unauthorized key wrapper.

RuntimeCredentialUnauthorizedAccountKey and account_key() have no call sites in the tree. They export a second subset shape for the marker without removing complexity elsewhere, which makes the new public API larger than the PR currently needs.

Fix: Remove RuntimeCredentialUnauthorizedAccountKey and account_key(); compare RuntimeCredentialUnauthorized directly until a real second consumer needs a reduced key type.

[henrypark133]

Closing in favor of a focused fix.

Root cause of the user-facing bug ("Could not save the token" on credential re-auth) is narrow: WASM/runtime auth_required 401 gates are emitted with empty credential_requirements (crates/ironclaw_host_runtime/src/services/runtime_adapters.rs:815 wasm_guest_dispatch_error). That leaves AuthPromptView.provider null, so the WebUI manual-token card (useChat.submitAuthToken) throws client-side before sending — the gate is structurally unsubmittable.

This PR addressed it via a heavy parallel pipeline (egress 401 marker → recovery egress → revoke/refresh → reauth bridge → HostRuntime AuthRequired override), which was also dead in practice: the scope guard compared the ephemeral invocation_id, so the recovery marker was always rejected.

The leaner fix — populate credential_requirements from the capability's already-declared credential obligation where auth_required is raised — resolves the same bug without the extra machinery. New PR to follow. Account revoke-on-401 can be revisited as a separate, smaller follow-up if needed.