Skip to content

fix(reborn): populate provider on runtime auth-required gates - #5180

Merged
henrypark133 merged 9 commits into
mainfrom
fix/reborn-reauth-gate-credential-requirements
Jun 25, 2026
Merged

henrypark133 merged 9 commits into
mainfrom
fix/reborn-reauth-gate-credential-requirements

Conversation

@henrypark133

Copy link
Copy Markdown
Collaborator

Problem

In the Reborn WebUI, when a runtime credential (e.g. a github PAT) returns 401, an "Authenticate to continue this run" gate appears — but pasting a fresh token shows "Could not save the token" and no network request is sent.

Root cause

The WASM/runtime auth_required path raises the gate with empty credential_requirements:

// crates/ironclaw_host_runtime/src/services/runtime_adapters.rs (wasm_guest_dispatch_error)
WasmGuestErrorKind::AuthRequired => DispatchError::AuthRequired {
    capability: capability.clone(),
    required_secrets: Vec::new(),
    credential_requirements: Vec::new(), // ← empty
},

Empty requirements → AuthPromptView.provider is None (auth_prompt.rs::auth_prompt_from_credential_requirement) → the WebUI manual-token card throws client-side (useChat.submitAuthToken: if (!runId || !gateRef || !provider) throw) before apiFetch. Hence the generic toast with no request. The credential-missing-at-dispatch path already populates the requirement; the 401-after-injection path was the gap.

Fix

In the capability host — where both the dispatch AuthRequired result and the capability's obligations are in scope — enrich an empty credential_requirements from the capability's already-declared credential obligations (InjectCredentialAccountOnce → RuntimeCredentialAuthRequirement { provider, setup, requester_extension, provider_scopes }). Wired at invoke_json and dispatch_resumed_capability.

  • Runtime-agnostic (WASM / MCP / Script).
  • Never overrides a populated list.
  • Reuses the same declared-requirement data the credential-missing path surfaces.
  • No new pipeline (this supersedes the closed Fix Reborn credential delete and same-run reauth #5174, which added an egress-marker → revoke/refresh → reauth-bridge stack for the same goal but was inert in practice).

Test

New caller-level regression crates/ironclaw_capabilities/tests/capability_host_auth_required_enrichment_contract.rs drives CapabilityHost::invoke_json with a capability that returns auth_required + a declared credential obligation, and asserts the gate carries provider=github (fails before the fix). Plus a non-override test for the MCP-style populated case.

Verification

  • cargo fmt clean
  • cargo clippy --all --tests --examples --all-features -- -D warnings → 0 warnings
  • cargo test -p ironclaw_host_runtime -p ironclaw_capabilities -p ironclaw_reborn_composition --lib passes (only Docker-socket env tests fail locally)

Still to confirm

Live end-to-end on deploy: gate now submittable → token saves → run resumes.

🤖 Generated with Claude Code

A WASM/runtime capability whose injected credential returns 401 raises an
`auth_required` gate with empty `credential_requirements`
(`runtime_adapters.rs` `wasm_guest_dispatch_error`). That left
`AuthPromptView.provider` null, so the WebUI manual-token card threw
client-side (`useChat.submitAuthToken` requires `provider`) and never sent
the submit — surfacing as "Could not save the token" with no network request.

Enrich an empty `DispatchError::AuthRequired.credential_requirements` from the
capability's already-declared credential obligations
(`InjectCredentialAccountOnce` -> `RuntimeCredentialAuthRequirement`) in the
capability host, where both the dispatch result and the obligations are in
scope. Runtime-agnostic; never overrides a populated list. This reuses the
same declared-requirement data the credential-missing path already surfaces,
so re-auth gates become submittable.

Adds a caller-level regression test driving `CapabilityHost::invoke_json`
that asserts the gate carries the provider (fails before the fix), plus a
non-override test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-5180 June 24, 2026 06:44 Destroyed
@github-actions github-actions Bot added size: M 50-199 changed lines risk: low Changes to docs, tests, or low-risk modules contributor: core 20+ merged PRs labels Jun 24, 2026
@coderabbitai

coderabbitai Bot commented Jun 24, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d621a831-d6e4-4292-af6f-ab947d35301c

📥 Commits

Reviewing files that changed from the base of the PR and between 5c8219a and 2bbe78e.

📒 Files selected for processing (2)
  • crates/ironclaw_capabilities/src/host.rs
  • crates/ironclaw_capabilities/tests/capability_host_auth_required_enrichment_contract.rs

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved durable auth reconnect handling: credential selection and manual token completion now succeed when only ephemeral request identifiers differ, while still enforcing ownership/session/surface isolation.
    • Auth-required dispatch failures are now enriched with a derived credential requirement when the dispatcher provides empty auth detail, improving auth-recovery guidance.
  • New Features

    • Exposed a way to derive credential auth requirements directly from credential injection obligations.
  • Tests

    • Added regression coverage for credential-enrichment behavior and its suppression in ambiguous/multi-obligation cases.
    • Added reconnect-focused durable-auth and binding-invariant unit/regression tests.
    • Updated runtime 401→auth recovery expectations to include exactly one derived OAuth requirement.

Walkthrough

Adds obligation-to-requirement mapping, enriches empty DispatchError::AuthRequired values in CapabilityHost, and changes durable auth completion to owner-granularity scope checks with regression coverage.

Changes

AuthRequired enrichment and durable auth scope

Layer / File(s) Summary
Obligation credential mapping
crates/ironclaw_host_api/src/decision.rs
Adds Obligation::credential_auth_requirement and tests that InjectCredentialAccountOnce maps to Some(RuntimeCredentialAuthRequirement) while other obligation variants return None.
Dispatch error enrichment
crates/ironclaw_capabilities/src/host.rs
Adds enrich_dispatch_error_credential_requirements, wires it into both dispatch failure paths in CapabilityHost, and tests empty-detail enrichment plus no-op cases.
CapabilityHost contract tests
crates/ironclaw_capabilities/tests/capability_host_auth_required_enrichment_contract.rs, crates/ironclaw_host_runtime/tests/github_wasm_runtime_contract.rs
Adds contract tests for invoke-time enrichment, resumed-path enrichment, preserved dispatcher requirements, multiple-obligation suppression, required-secret preservation, and updated runtime auth gate expectations.
Durable auth scope validation
crates/ironclaw_auth/src/fakes.rs, crates/ironclaw_reborn_composition/src/product_auth_durable/flows.rs
Replaces full-scope equality with binding_scope_owns_account in durable credential-selection and manual-token completion paths.
Durable auth scope regression tests
crates/ironclaw_auth/src/credential.rs, crates/ironclaw_reborn_composition/src/product_auth_durable/tests.rs
Adds binding-scope ownership tests and durable completion regressions for invocation_id drift, owner mismatches, session mismatches, and auth-surface mismatches.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • nearai/ironclaw#4839: Shares the CapabilityHost auth/resume dispatch path that now enriches resumed DispatchError::AuthRequired values.
  • nearai/ironclaw#4969: Uses the same AuthRequired credential-requirement shape now asserted by the Google Drive 401 tests.

Suggested reviewers

  • aiworkbot
  • zetyquickly

Poem

A gate once mute found one clear thread,
From obligation maps a reason spread.
Owners drifted, yet the bound held fast,
And 401s learned to speak at last.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The change addresses auth-required enrichment, but it does not cover several #5174 requirements like credential delete, marker comparison, or bridge ownership. Add or link the missing issue-scope changes for delete-route handling, RuntimeCredentialUnauthorized comparison, and reauth-bridge ownership, or split the PR.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title is conventional-commit style and accurately describes the main change: enriching runtime auth-required gates with a provider.
Description check ✅ Passed The description covers the problem, root cause, fix, tests, and verification, so it is mostly complete despite not matching the template exactly.
Out of Scope Changes check ✅ Passed All file changes support the auth-recovery and enrichment flow; no unrelated subsystems or feature work stand out.

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


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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@crates/ironclaw_capabilities/src/host.rs`:
- Line 1919: The auth resume enrichment in CapabilityHost’s shared resume
dispatch is only covered indirectly today; add a test that exercises the real
caller path through resume_json or auth_resume_json, not invoke_json. Use the
existing auth resume flow to reach the branch where
enrich_auth_required_from_obligations is applied, and assert that an
AuthRequired with empty credential_requirements gets populated from the
obligations. Keep the test anchored to CapabilityHost and the resumed auth gate
behavior so the same-run reauth path is covered.
🪄 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: e985e13e-6dd1-4aa9-8978-8a96e0438e8b

📥 Commits

Reviewing files that changed from the base of the PR and between 42307a7 and 9c095b4.

📒 Files selected for processing (2)
  • crates/ironclaw_capabilities/src/host.rs
  • crates/ironclaw_capabilities/tests/capability_host_auth_required_enrichment_contract.rs

Comment thread crates/ironclaw_capabilities/src/host.rs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9c095b4275

ℹ️ 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".

Comment thread crates/ironclaw_capabilities/src/host.rs Outdated

@henrypark133 henrypark133 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review (multi-agent)

Intent: Fix the Reborn runtime auth-required flow so gates created after a 401 include the credential provider from declared obligations, allowing the WebUI to submit a fresh token and resume the run.

Stats: 2 findings (from 2 raw, 2 after dedup) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 1

Tests

  1. Medium No regression test for auth-resume provider enrichment (crates/ironclaw_capabilities/src/host.rs:1916-1924, confidence 86) — anchor: crates/ironclaw_capabilities/src/host.rs:1919
    The new regression test only drives invoke_json, but dispatch_resumed_capability also enriches empty credential_requirements from obligations. There is no test asserting that auth_resume_json preserves the provider on an AuthRequired bounce, so this resumed flow could still regress without detection.

  2. Low Missing many-obligation coverage for credential requirement collection (crates/ironclaw_capabilities/src/host.rs:2273-2304, confidence 74) — anchor: crates/ironclaw_capabilities/src/host.rs:2293 (no diff position — body only)
    The helper folds all matching InjectCredentialAccountOnce obligations into credential_requirements, but the added test only covers a single obligation. A capability with multiple injected credentials would still depend on this code, and the suite would not catch dropping or mis-ordering additional requirements.

Comment thread crates/ironclaw_capabilities/src/host.rs Outdated
@railway-app

railway-app Bot commented Jun 24, 2026 •

Copy link
Copy Markdown

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

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

…pes; fix take-one

Address code-review findings on the runtime auth-required enrichment:

- Correctness: emit at most one credential requirement. The downstream
  consumer (auth_prompt_from_credential_requirement) matches exactly one
  (`let [requirement] = ...`); emitting >1 for capabilities with multiple
  credential obligations made it fall through and leave the gate
  unsubmittable. Enrich with `.take(1)`.
- Altitude/duplication: move the logic onto the types that own it in
  ironclaw_host_api — `Obligation::credential_auth_requirement()` and
  `DispatchError::enrich_auth_requirements(&[Obligation])`. Delete the
  free helper from the capability host (host.rs shrinks; the two call
  sites become one-liners and can't drift).
- Tests: add resume-path coverage (auth_resume_json -> dispatch_resumed_
  capability), a multi-obligation test locking the take-one contract, and
  host_api unit tests for both new methods.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-5180 June 24, 2026 07:10 Destroyed
@github-actions github-actions Bot added size: L 200-499 changed lines and removed size: M 50-199 changed lines labels Jun 24, 2026
… completion

Folds the second link of runtime credential re-auth into this PR.

Manual-token (and credential-selection) submit mints a fresh per-request
`invocation_id`, so completing a flow that reconnects to a credential account
created in an earlier flow failed `scope_matches` full-equality with
CrossScopeDenied (HTTP 403) — the "Could not save the token" follow-on once the
gate became submittable. This is #4935 defect A on the unbound/reusable path.

- `complete_manual_token` and `complete_credential_selection`
  (product_auth_durable/flows.rs) now use `binding_scope_owns_account`:
  owner-granularity (tenant/user/agent/project hard-required, session + surface
  exact-matched) while ignoring the ephemeral invocation_id (and thread/mission,
  intentional for owner-reusable accounts).
- Mirror the same fix in the in-memory fake (fakes.rs) so it cannot mask the
  divergence in unit tests.
- Tests: cross-invocation reconnect succeeds (both paths); genuinely foreign
  owner still rejected; cross-session and cross-surface still rejected
  (path-partitioned on disk).

Follow-ups (not in this PR): rename `binding_scope_owns_account` ->
`scope_owns_account` (now used on unbound paths too); extract a shared
completion-account validation helper to unify the three call sites.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-5180 June 24, 2026 07:32 Destroyed
@github-actions github-actions Bot added size: XL 500+ changed lines and removed size: L 200-499 changed lines labels Jun 24, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@crates/ironclaw_reborn_composition/src/product_auth_durable/tests.rs`:
- Around line 3748-3822: The current regression test only proves a cross-session
lookup can fail at the filesystem layer and still pass with CredentialMissing,
so it does not force the new binding_scope_owns_account check to run. Update
filesystem_complete_manual_token_rejects_different_session_id (and the related
surface test) to use complete_manual_token with an account that is reachable but
whose account.scope differs from the flow scope, so the call site actually loads
the account and hits binding_scope_owns_account. Then assert CrossScopeDenied
for the mismatched scope axis instead of accepting only CredentialMissing,
keeping the test focused on the real caller behavior rather than the helper-only
path.
🪄 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: d3fa7bc7-357d-4e0a-8238-10d7ca1afd4f

📥 Commits

Reviewing files that changed from the base of the PR and between de0bb11 and 7c8333d.

📒 Files selected for processing (3)
  • crates/ironclaw_auth/src/fakes.rs
  • crates/ironclaw_reborn_composition/src/product_auth_durable/flows.rs
  • crates/ironclaw_reborn_composition/src/product_auth_durable/tests.rs

@serrrfirat serrrfirat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-agent review + thermonuclear maintainability pass for 7c8333deccf218aa09c88b2e5ba4abee13447344.

Reviewers:

  • intent: high-confidence goal extraction
  • security: 0 findings
  • bugs: 1 finding
  • performance/concurrency: 0 findings
  • tests: 1 kept finding, 1 low overlapping edge-case dropped during dedupe
  • conventions/thermo: 1 structural finding

I’m leaving this as COMMENT rather than REQUEST_CHANGES because the kept findings are Medium severity, but the first two should be addressed before merge.

Comment thread crates/ironclaw_host_api/src/dispatch.rs Outdated
Comment thread crates/ironclaw_host_api/src/dispatch.rs Outdated
@henrypark133

Copy link
Copy Markdown
Collaborator Author
Screenshot 2026-06-24 at 8 08 51 AM verified on railway

…ghten condition

Address PR #5180 review (thermo-nuclear + multi-agent):

- Altitude: keep the neutral `Obligation::credential_auth_requirement` mapper
  in ironclaw_host_api, but move the enrichment POLICY out of
  `DispatchError::enrich_auth_requirements` (product-workflow cardinality has no
  place in the neutral vocab crate per its guardrail) into a private helper in
  ironclaw_capabilities.
- Correctness: synthesize the auth-gate credential requirement ONLY when the
  runtime gave no auth signal of its own (both `required_secrets` and
  `credential_requirements` empty) AND the capability declares EXACTLY ONE
  credential obligation. Raw-secret gates (required_secrets populated) are no
  longer mis-prompted as product-auth; multi-credential capabilities no longer
  get a wrong-provider gate (was `.take(1)` guessing the first).
- Tests: unit tests for all helper branches; updated the multi-obligation
  contract test to assert the gate is left unmodified (empty) rather than
  pointed at an arbitrary provider.

Also adds durable rejection coverage for complete_credential_selection
(foreign owner reaches binding_scope_owns_account -> CrossScopeDenied; session/
surface are path-partitioned -> CredentialMissing, guard exact-match is
defense-in-depth).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-5180 June 24, 2026 15:32 Destroyed
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-5180 June 24, 2026 18:11 Destroyed
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-5180 June 24, 2026 21:01 Destroyed

@henrypark133 henrypark133 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review (multi-agent)

Intent: Populate missing provider metadata on runtime auth-required gates so the WebUI token prompt can save credentials and resume runs.

Stats: 2 findings (from 2 raw, 2 after dedup) across 2 files. Reviewers run: security, performance, tests, local-patterns, maintainability. Reviewers failed: bugs, conventions, approach (gpt-5.4-mini capacity). Body-only: 0

Local Patterns

  1. Low Update the stale helper name in the test header comment (crates/ironclaw_capabilities/tests/capability_host_auth_required_enrichment_contract.rs:14-17, confidence 97) — anchor: crates/ironclaw_capabilities/tests/capability_host_auth_required_enrichment_contract.rs:16
    The file-level comment still tells readers to look at enrich_auth_required_from_obligations, but the actual helper is named enrich_dispatch_error_credential_requirements. That stale symbol reference makes the comment misleading and breaks normal grep/navigation for the fix path.

  2. Low Avoid hard-coding a line number in the scope comment (crates/ironclaw_auth/src/fakes.rs:308-313, confidence 82) — anchor: crates/ironclaw_auth/src/fakes.rs:309-312
    The new comment bakes in credential.rs:580, which will go stale as soon as that file shifts and leaves a dead line reference behind. That makes the explanation harder to trust than a symbol-based pointer.

Comment thread crates/ironclaw_auth/src/fakes.rs
…on/surface

Addresses the CI failure and the remaining review feedback on the
credential_requirements enrichment PR.

- github_wasm_runtime_contract.rs: the two google-drive WASM 401 tests
  asserted credential_requirements.is_empty() — the pre-fix, un-wired
  contract from #4969 (provider-null, unsubmittable gate, #5174). The
  enrichment now populates the gate from the single credential
  obligation, so assert one requirement with provider=google + OAuth
  setup. This is the runtime-401 re-auth fallback; proactive refresh
  (inline + background keepalive) already runs before injection.

- host.rs: document the reactive-refresh-on-runtime-401 follow-up on the
  enrichment helper, and correct the downstream-consumer note (OAuth
  setup launches the OAuth flow, ManualToken renders the token card).

- credential.rs: add direct unit tests for binding_scope_owns_account
  covering the session_id and surface exact-match branches. The durable
  filesystem caller tests partition account records by surface+session
  path, so those axes only ever returned CredentialMissing and never
  executed the guard's equality branches (coderabbit review point).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-5180 June 25, 2026 04:19 Destroyed
Address two low-severity review nits:
- capability_host_auth_required_enrichment_contract.rs: header referenced
  the old helper name enrich_auth_required_from_obligations; rename to the
  real enrich_dispatch_error_credential_requirements.
- fakes.rs / flows.rs: scope comments hard-coded credential.rs:580, which
  is already stale (binding_scope_owns_account is now at line 607). Drop
  the line number and point at the symbol only.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-5180 June 25, 2026 04:21 Destroyed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@crates/ironclaw_capabilities/src/host.rs`:
- Around line 2276-2284: The doc comment in the OAuth gate/fallback logic
overstates cross-layer refresh guarantees that are owned outside this module.
Update the comment near the OAuth credential handling in host.rs to either
explicitly cite the external injection/keepalive contract or soften the wording
to describe intended behavior and known limitations. Keep the description
aligned with the actual behavior of the gate/fallback path so it does not
promise enforced refresh behavior that `ironclaw_capabilities` does not
guarantee.
🪄 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: 227587c3-057e-426b-8db0-4b4605f95171

📥 Commits

Reviewing files that changed from the base of the PR and between ee84f89 and 0cfe1a2.

📒 Files selected for processing (3)
  • crates/ironclaw_auth/src/credential.rs
  • crates/ironclaw_capabilities/src/host.rs
  • crates/ironclaw_host_runtime/tests/github_wasm_runtime_contract.rs

Comment thread crates/ironclaw_capabilities/src/host.rs

@henrypark133 henrypark133 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review (multi-agent)

Intent: Fix runtime auth-required gates by enriching empty credential requirements so manual token submission works after 401 responses.

Stats: 1 finding (from 2 raw, 1 after validation) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.

Tests

  1. Medium Add caller-level coverage for raw-secret auth gates (crates/ironclaw_capabilities/src/host.rs:2297-2305, confidence 84) — anchor: crates/ironclaw_capabilities/src/host.rs:2297
    The new enrichment helper correctly bails out when required_secrets is already populated, but the caller-level contract tests only exercise empty-secret AuthRequired responses. The raw-secret preservation case is covered only at the private helper level, so a future wiring regression in CapabilityHost::invoke_json or auth_resume_json could still rewrite a raw-secret gate into a provider prompt.

Comment thread crates/ironclaw_capabilities/src/host.rs
…h doc

Address two review nits on the enrichment helper:
- host.rs: the OAuth runtime-401 follow-up doc stated cross-layer refresh
  (inline injection + background keepalive) as a guarantee, but those live
  in other crates and are not enforced here. Soften to "may already have
  been attempted".
- capability_host_auth_required_enrichment_contract.rs: add
  invoke_json_preserves_required_secrets_from_dispatcher — a caller-level
  test driving CapabilityHost::invoke_json with a raw-secret AuthRequired
  (required_secrets populated, credential_requirements empty) while an
  InjectCredentialAccountOnce obligation is declared, asserting the gate is
  left unmodified (secrets preserved, not rewritten into a provider prompt).
  Previously covered only at the private-helper level.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-5180 June 25, 2026 04:55 Destroyed
@henrypark133
henrypark133 merged commit cecd958 into main Jun 25, 2026
119 checks passed
@henrypark133
henrypark133 deleted the fix/reborn-reauth-gate-credential-requirements branch June 25, 2026 05:18
henrypark133 added a commit that referenced this pull request Jul 4, 2026
… pack, triggered auth delivery, attachments, golden/synthetic expansions (#5610)

* test(reborn): doc/text + multi-attachment coverage (W4-ATTACH-VARIANTS)

Adds submit_turn_with_attachments (generalizes the image-only
submit_turn_with_image_attachment to N attachments of any mime type)
and two int-tier tests: a text/plain attachment's extracted text
reaching the model, and two attachments in one turn both reaching the
model with distinct index ordinals. Closes the doc/multi-attachment
gap in C-ATTACH (only single-image coverage existed before).

* test(reborn): W4-AUTHGATE-WIRE — runtime-401 provider-gate + cancel-no-replay (wave-4 row 1)

Pins the #5174/#5180 bug class (empty credential_requirements leaving
AuthPromptView.provider null, "Could not save the token" with no network
request) through the FULL scripted-gateway integration harness — a tier
below the existing crate-level pins, which drive CapabilityHost::invoke_json
or HostRuntimeServices::invoke_capability directly and never exercise the
real submit_turn -> BlockedAuth wire the WebUI depends on.

- tests/reborn_integration_auth_gate.rs: new
  runtime_401_after_injection_populates_provider_credential_requirement
  (github credential resolves OK but the runtime HTTP call 401s; asserts
  the resulting BlockedAuth gate's credential_requirements carries
  provider=github + ManualToken setup), cancel_blocked_auth_gate_leaves_no_stale_replay
  (cancelling a BlockedAuth run lands directly on Cancelled with no active
  worker, and the SAME real gate ref can no longer resume it afterward —
  closes the #5067/#4957 class of gates staying "live"), and
  deny_auth_gate_rejects_a_non_auth_gate_ref_prefix (negative companion).
  Flip-check: temporarily bypassed the host.rs enrichment call site,
  confirmed the flagship test fails with the exact pre-fix empty-list
  shape, restored (crates/ironclaw_capabilities/src/host.rs left
  byte-identical — no production diff).

- tests/support/reborn/harness.rs: RecordingNetworkHttpEgress gains an
  additive FIFO status_queue (default empty -> unchanged hardcoded-200
  behavior) + install_network_status_script accessor. Needed because
  GithubIssueTools' real WASM HTTP call flows through the network-egress
  lane, not the runtime-egress lane the existing ScriptedHttpResponse
  matcher scripts (try_with_host_http_egress overwrites the runtime port —
  see reborn_integration_secret_injection.rs's module doc) — the prior
  double had no way to script a non-200 status on that lane at all.
- tests/support/reborn/builder.rs: with_github_network_status(status)
  builder method (FIFO) threading github_network_statuses through
  RebornCapabilityBackend::install.
- tests/support/reborn/capability_backend.rs: wires keyed_http_responses
  (previously dropped for this backend) and the new github_network_statuses
  into the GithubIssueTools install arm; no-op for existing empty-vec callers.
- tests/support/reborn/assertions.rs: assert_network_egress_count, sibling
  of assert_egress_count for the network-lane call-count proofs above.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(reborn): unknown extension_id fails extension_install safely (W4-EXT-MANIFEST-ERR)

Narrowed from the originally-scoped manifest-content arms (schema
mismatch/reserved id/forbidden trust level): extension_install's only
input is a catalog-resolved extension_id over a fixed,
compile-time-embedded bundled catalog, so raw manifest TOML never
reaches ManifestV2Error validation through this capability in
production. The one reachable, wired arm is an unknown extension_id,
which fails Failed{invalid_input} rather than panicking or no-oping.

* test(reborn): W4-PROVIDER-VALIDATE — password/traceback caller-gap coverage

#5001 (PinchBench bucket D) removed the crude SENSITIVE_PROVIDER_TEXT_MARKERS
substring scan on provider reasoning/response_reasoning/signature text (bare
words like "password"/"traceback" were false-positive-rejected, driving
retry/give-up loops); the entropy-based LeakDetector is the real guard now.
That contract was pinned only at the private free-function level
(capability_port/provider_validation.rs's own unit test calling
validate_provider_tool_call directly) — the #5001 caller gap.

Adds provider_tool_call_registration_accepts_password_and_traceback_reasoning_text
in crates/ironclaw_loop_support/src/capability_port.rs's existing test module,
alongside the crate's other caller-level `port.validate_provider_tool_call(&call)`
tests: drives the REAL production caller
(LoopCapabilityPort::validate_provider_tool_call / register_provider_tool_call
/ invoke_capability on HostRuntimeLoopCapabilityPort, the same port the agent
loop calls) with "password"/"traceback" in all three metadata fields, and
proves genuine acceptance through to a real Completed dispatch (not just a
non-error return).

Flip-check: temporarily bloated response_reasoning past
PROVIDER_METADATA_TEXT_MAX_BYTES to confirm the assertion mechanism
discriminates a genuine rejection (fails with the expected
"exceeds 16384 bytes" error), then restored the password/traceback content.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(reborn): W4-MCP-SSO-WIRING — NEAR AI host-managed fallback through build_reborn_services

#5439 fixed NEAR AI MCP token resolution for SSO users: a Google-SSO user in
the same tenant/agent as the boot owner, with no NEAR AI token of their own,
now falls back to the host-managed (boot-owner) NEAR AI credential instead of
being prompted for one. That contract was pinned only at the private
rule/selector level (product_auth_runtime_credentials/tests.rs never calls
build_reborn_services) — the composition-wiring gap this row targets.

Adds local_dev_nearai_runtime_selection_falls_back_to_host_managed_account_for_sso_user
to extension_lifecycle_capabilities_auth_tests.rs (extending the existing
in-crate #[cfg(test)] composition-test file — same pattern as the sibling
github manual-token test above, template: product_auth_refresh_composition.rs's
"drive build_reborn_services directly" style). Drives ONLY the public surface:
build_reborn_services (local-dev always derives nearai_mcp_host_managed_scope
from the boot owner, so no live NEAR AI config injection is needed) plus the
crate-internal runtime_credential_account_selection_service() accessor this
file already had precedent for calling. Two discriminating arms on one
composed `services`: an SSO user in the owner's tenant/agent (different
project -- local-dev's host scope is project-unscoped by design) resolves via
fallback; an SSO user under a different tenant does not (CredentialMissing) --
proving the positive arm is a real scope match, not the selector always
succeeding.

Flip-check: temporarily short-circuited
RebornProductAuthServices::runtime_credential_account_selection_service to
always return the un-decorated selector (pre-#5439 behavior), confirmed the
new test's positive arm fails with CredentialMissing, restored (auth.rs left
byte-identical -- no production diff).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(reborn): C-SYNTH deferred arms — AmbiguousSkill seeding + project_create fault-injection (wave-4 lane C)

Two carry-over arms deferred from wave-3 PR #5584:

- skill_activate AmbiguousSkill: seed a system-scoped AND a user-scoped
  skill sharing one name (both SkillTrust::Trusted per
  FilesystemSkillBundleRoot::system/user) so the real
  validate_explicit_mentions_are_unambiguous reject path fires end-to-end,
  not just at the skill_activation.rs unit-test level. New
  seed_user_skill_for_test harness helper (additive, mirrors
  seed_system_skill_for_test).

- project_create fault-injection: new FaultInjectingProjectService test
  double (project_service_fault.rs) wrapping the real ProjectService at the
  production-wired Arc<dyn ProjectService> seam, forcing
  ProjectServiceError::Denied for a sentinel project name and delegating
  everything else to the real store. New
  project_tools_with_fault_injection()/project_lifecycle_fault_injected()
  harness+group constructors (additive).

  Deliberately NOT ProjectServiceError::Unavailable/Internal: investigation
  found both route through DefaultRecoveryStrategy's capability-retry
  branch, whose retry re-dispatch hits a real, confirmed production bug for
  provider-tool-call-originated invocations under local-dev composition —
  LocalDevCapabilityIo::resolve_capability_input rejects the reused
  input_ref on the retry with InvalidInvocation/"capability input ref was
  not staged for this loop run", collapsing the documented "retry twice,
  then a model-visible Failed" contract into an immediate terminal
  driver_unavailable. Documented in project_service_fault.rs; reported
  separately (not fixed — production change, out of this lane's scope).

Both flip-checked (mutated seed/fault-injection to prove discriminating
failure) and reverted before commit.

* test(reborn): golden payload expansions — parallel tool_calls, image attachment, gated-turn resume (wave-4 lane C)

Three scenario expansions to tests/reborn_integration_golden_payload.rs
(carry-over from wave-3 PR #5584):

- golden_parallel_tool_calls: new RebornScriptedReply::tool_calls([..])
  constructor (additive to reply.rs) scripts ONE assistant response with
  TWO tool_calls[] entries, pinning that multiple calls in one turn each
  get a distinct id and each following tool-role message's tool_call_id
  lines up in order — a shape the existing single-call golden_tool_call_feedback
  can't exercise.

- golden_image_attachment_turn: an inline image landed through the real
  submit_inbound_with_attachments entry point
  (RebornIntegrationGroup::attachment_tools()), routed through a
  vision-pattern model id, pinning the multimodal ContentPart::ImageUrl
  data: URL alongside the text part byte-for-byte.

- golden_gated_turn_approve: a real BlockedApproval gate raised, approved,
  and resumed (RebornIntegrationGroup::live_approvals()), snapshotting BOTH
  inference calls around the gate — proving the resume doesn't drop,
  duplicate, or reorder accumulated turn history.

Two normalization fixes to golden.rs, both needed for these scenarios to be
reproducible (discovered while authoring, not pre-existing regressions):

- Attachment-landing scenarios embed today's real UTC date in the landed
  project path (chrono::Utc::now(), no test seam) — added a second
  <DATE> filter alongside the existing loop-start-clock <TIMESTAMP> filter,
  or the image golden would bit-rot on every day boundary.

- Tool-call ids come from a NEXT_TOOL_CALL_ID counter shared by every test
  in this one compiled binary; running more than one tool-call-scripting
  golden test concurrently (the default `cargo test` thread pool) makes the
  raw id values order-dependent. Added normalize_tool_call_ids: renumbers
  every call-<N> to a canonical call-1, call-2, … in order of first
  appearance per rendered payload, preserving the id/tool_call_id linkage
  the golden actually cares about without depending on the racy raw value.
  Confirmed behavior-preserving for the four pre-existing snapshots (no
  diff) and confirmed the race is fixed (5 consecutive full-suite green
  runs). Flip-checked (forced two parallel tool_calls to share one id;
  golden correctly failed) and reverted before commit.

* test(reborn): W4-ASK-EACH-ONCE — ask-each-time approval resumes exactly once

#5306 fixed an unresumable BlockedApproval loop: require_approval_for_profile_policy
checked the explicit ask_each_time override (and the hard-floor force-approval
class) BEFORE consulting the matching one-shot approval lease a resume
carries, so an approved AskEachTime-gated resume re-hit the ask_each_time
branch and re-gated instead of completing. Only a Python E2E test
(test_tool_approval.py) exercised this class before; no Rust harness
coverage existed.

Adds scenario_ask_each_time_resumes_once.rs to the reborn_group_approvals
binary (both approvals_group_e2e and its libsql variant), run LAST because it
installs a persistent, group-wide ToolPermissionOverride::AskEachTime
override on builtin.write_file that would force-gate every sibling
scenario's plain-Ask-mode writes. Submits under the override, approves the
resulting BlockedApproval gate, and proves the resume reaches Completed in
ONE round trip with the write actually persisted — plus a companion
"resumes exactly once" proof that re-approving the same now-resolved gate_ref
fails NotPending (not a fresh re-raised gate).

tests/support/reborn/harness.rs: adds a generic
tool_permission_overrides: Option<Arc<dyn ToolPermissionOverrideStore>> field
(mirrors the existing auto_approve_settings field's pattern — populated only
by new_with_options, None elsewhere) and
set_ask_each_time_override_for_test, generalizing
disable_outbound_target_set_tool's override-store access beyond
outbound_target_tools() to any host-runtime-backed harness/group.

Flip-check: temporarily restored the pre-#5306 check order in
profile_approval_authorization.rs (ask_each_time/hard-floor before the
one-shot lease), confirmed the new scenario fails (the approved write never
persists), restored (file left byte-identical — no production diff).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(reborn): triggered-origin chained gated journey (wave-4 lane C)

Carry-over from wave-3 PR #5584: a triggered fire whose run raises a
BlockedApproval gate, gets resolved, then CHAINS into a SECOND
BlockedApproval gate in the SAME run (the post-resume model call issues
another gated tool call instead of finalizing), driven through
submit_triggered_turn_scripted (E-TRIGGERED-SUBMIT).

New scenario_triggered_chained_gate::run_chained_approve, registered as its
own live_approvals group in reborn_group_triggers::triggered_gate_group.
Re-reads TurnOriginKind::ScheduledTrigger fresh at the coordinator boundary
at THREE checkpoints (first park, second/chained park, final Completed) —
not just trusting the initial TriggeredSubmission — closing the gap that a
resume path rebuilding product_context from a non-trigger-aware default on
the SECOND hop would otherwise slip through undetected. Also asserts both
gate_refs are genuinely distinct, both chained writes persisted, and the
final reply persisted in the trigger's own thread.

Flip-checked (asserted the wrong origin kind; the checkpoint helper
correctly failed with the real ScheduledTrigger value in the diagnostic)
and reverted before commit.

Also folds in `cargo fmt` whitespace-only fixes surfaced while formatting
this new file (golden.rs, harness.rs, reply.rs, and two golden/skill-activate
test files touched by prior lane-C commits) — no semantic change, reran
their test bins green after formatting.

* test(reborn): extract trigger-prompt materializer test-support helper (wave-4 lane C)

Committed follow-up on PR #5584's review thread: submit_triggered_turn_scripted
hand-mirrored ConversationContentRefMaterializer::materialize_prompt
(trigger_resolve_request + record_trigger_prompt + the content-ref shape,
field-by-field) instead of reusing it, and — as flagged — deliberately
SKIPPED authorize_trigger_fire and validate_trusted_trigger_prompt. Flagged
as a drift trap (trusted-trigger materialization is an ownership boundary,
AGENTS.md:61); the review agreed the fix is a #[cfg(feature = "test-support")]
materializer helper returning (TriggerMaterializedPrompt, TurnScope) living
beside the real materializer, held out of #5584 as a fast-follow with this
exact shape.

New production-crate (test-support-gated, compiles out of default builds)
surface in ironclaw_reborn_composition:
- trigger_poller_trusted_submit.rs: materialize_trigger_prompt_for_test,
  #[cfg(any(test, feature = "test-support"))] — runs the REAL production
  pipeline via ConversationContentRefMaterializer::materialize_prompt
  (authorize + validate + resolve + record + content-ref), then an
  idempotent second resolve_or_create_binding_with_trusted_scope call (safe
  — same request, same already-created binding) to also return the
  TurnScope the trait method computes internally but never exposes. Plus
  two crate-tier unit tests: positive (returned scope/content-ref match an
  independent ground-truth resolve) and negative (an unsafe prompt is
  rejected by the REAL safety validator).
- test_support/trigger_materializer.rs: pub, feature="test-support"-gated
  thin wrapper re-exported from test_support/mod.rs — the established
  wrap_project_create_capability_for_test-style pattern.

tests/support/reborn/triggered_submit.rs: submit_triggered_turn_scripted now
calls this ONE production-owned helper instead of hand-mirroring; deletes
~90 net lines of duplicated resolve/thread-record/content-ref logic.

Verified default-features build of ironclaw_reborn_composition stays
warning-free (function/import correctly compile out). Flip-checked at the
INTEGRATION level (not just the new crate-unit tests): forced an
injection-pattern prompt through submit_triggered_turn_scripted — every
triggered-gate scenario correctly failed with "rejected by safety scan",
proving the old hand-mirrored path's skip of validate_trusted_trigger_prompt
is now closed. Reverted before commit. All touched integration test bins
(reborn_group_triggers, reborn_integration_triggered_submit, plus every
other wave-4 lane-C bin) rerun green after the extraction.

* test(reborn): W4-TRIGSLACK-SETTLE — auth-gate coverage for TriggeredRunDeliveryDriver

TriggeredRunDeliveryDriver was exercised by exactly one crate-tier test
(triggered_approval_prompt_route_resolves_dm_approve_on_foreign_scope),
covering only the approval-gate path. Add the auth-gate twin: a
BlockedAuth triggered run whose auth-prompt preference resolves to the
creator's DM must carry the OAuth setup link
(triggered_auth_prompt_route_delivers_dm_setup_link_on_foreign_scope),
mirroring slack_dm_delivers_auth_prompt_with_setup_link_after_immediate_ack's
assertion shape but driven through the real triggered-delivery driver.

TriggeredRunDeliveryDriver only ever targets the creator's personal DM
(never a channel), so there is no literal "channel" arm to mirror
slack_channel_auth_prompt_omits_setup_link_after_immediate_ack. The
discriminating negative arm instead exercises the driver's own
send-time OAuth-DM backstop
(triggered_auth_prompt_oauth_target_not_dm_suppresses_setup_link_and_cancels_run):
when the resolved auth-prompt target is not a personal DM, the setup
link must never be posted and the blocked run must be cancelled
instead.

ScriptedTriggerCoordinator gains an additive
new_with_first_poll constructor (script an arbitrary first-poll
status/gate_ref instead of the hardcoded BlockedApproval/GATE pair)
and a functional cancel_run (previously unreachable!, since the
approval-only scenario never called it) to support the OAuth-not-DM
arm. Test code only; no production changes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* test(reborn): extract trigger-prompt materializer test-support helper (wave-4 lane C)

Committed follow-up on PR #5584's review thread: submit_triggered_turn_scripted
hand-mirrored ConversationContentRefMaterializer::materialize_prompt
(trigger_resolve_request + record_trigger_prompt + the content-ref shape,
field-by-field) instead of reusing it, and — as flagged — deliberately
SKIPPED authorize_trigger_fire and validate_trusted_trigger_prompt. Flagged
as a drift trap (trusted-trigger materialization is an ownership boundary,
AGENTS.md:61); the review agreed the fix is a #[cfg(feature = "test-support")]
materializer helper returning (TriggerMaterializedPrompt, TurnScope) living
beside the real materializer, held out of #5584 as a fast-follow with this
exact shape.

New production-crate (test-support-gated, compiles out of default builds)
surface in ironclaw_reborn_composition:
- trigger_poller_trusted_submit.rs: materialize_trigger_prompt_for_test,
  #[cfg(any(test, feature = "test-support"))] — runs the REAL production
  pipeline via ConversationContentRefMaterializer::materialize_prompt
  (authorize + validate + resolve + record + content-ref), then an
  idempotent second resolve_or_create_binding_with_trusted_scope call (safe
  — same request, same already-created binding) to also return the
  TurnScope the trait method computes internally but never exposes. Plus
  two crate-tier unit tests: positive (returned scope/content-ref match an
  independent ground-truth resolve) and negative (an unsafe prompt is
  rejected by the REAL safety validator).
- test_support/trigger_materializer.rs: pub, feature="test-support"-gated
  thin wrapper re-exported from test_support/mod.rs — the established
  wrap_project_create_capability_for_test-style pattern.

tests/support/reborn/triggered_submit.rs: submit_triggered_turn_scripted now
calls this ONE production-owned helper instead of hand-mirroring; deletes
~90 net lines of duplicated resolve/thread-record/content-ref logic.

Verified default-features build of ironclaw_reborn_composition stays
warning-free (function/import correctly compile out). Flip-checked at the
INTEGRATION level (not just the new crate-unit tests): forced an
injection-pattern prompt through submit_triggered_turn_scripted — every
triggered-gate scenario correctly failed with "rejected by safety scan",
proving the old hand-mirrored path's skip of validate_trusted_trigger_prompt
is now closed. Reverted before commit. All touched integration test bins
(reborn_group_triggers, reborn_integration_triggered_submit, plus every
other wave-4 lane-C bin) rerun green after the extraction.

* test(reborn): review fixes — consolidate slack e2e poll helpers, cite #5608 in fault-injection rationale

Factor the three near-identical bounded-poll-for-chat.postMessage
helpers in slack_serve/e2e_tests.rs into one predicate-parameterized
wait_for_post_messages_matching, and replace "Lane C final report"
citations with the filed issue (#5608) in the local-dev retry-path
rationale comments.

* test(reborn): address wave4 review comments

* test(reborn): relax auth gate harness wait

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

This branch was successfully deployed

No deployments
ironclaw-ci-preview / ironclaw-pr-5180 — 2bbe78ef Deployed Jun 25, 2026 by railway-app[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: low Changes to docs, tests, or low-risk modules size: XL 500+ changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants