fix(webui): avoid token prompt for OAuth auth gates - #4957
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesOAuth Auth Prompt Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
github-actions
Bot
added
size: M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the auth prompt handling to support OAuth credential requirements alongside manual tokens, ensuring OAuth prompts are not downgraded to manual token prompts. It includes corresponding backend and frontend test coverage. The review feedback suggests optimizing string cloning in the Rust backend and simplifying the logic for detecting modern challenge fields in the JavaScript frontend.
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.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Collaborator
|
Human final review guide for #4957: CI is green and GitHub reports Please focus final review on:
Automation verification performed:
|
think-in-universe
approved these changes
Jun 16, 2026
This was referenced Jun 17, 2026
This was referenced Jun 17, 2026
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
* fix(webui): avoid token prompt for oauth auth gates * chore(webui): address oauth auth gate review comments
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #4884.
Testing