Repository navigation
fix(reborn): honest extension connection state across search, surface, and model (#5416) - #5528
henrypark133 wants to merge 2 commits into
Conversation
…, and model (#5416) Fixes the contradictory Google/Gmail authentication flow where the agent claimed an extension was "already configured or active — do not ask the user for credentials" while no credential account existed, then hit an auth gate on first use. Third patch on this oscillating predicate (#4996 -> #5037 -> here); this one replaces the predicate class instead of re-tuning it. Phase 0 — collapsed model-facing availability (search surface). - New `ExtensionAvailability` enum (available / needs_auth / not_installed / unknown) replaces the leaked lifecycle `installation_phase` on `LifecycleSearchExtensionSummary`; projected from installed? x credential-readiness in the new `extension_availability` composition module. Lifecycle phase stays internal. - Deletes `suppress_search_credential_onboarding` / `search_installation_phase` / `search_credentials_configured`: the "ready" message now requires a genuinely Available projection, and credential requirements/onboarding stay on non-Available results. - Defect B: settings `extension_info` fails closed on Unknown credential readiness (authenticated=false, needs_setup=true) with a new gated ReverifyRequired onboarding state for plausibly-connected phases only. Phase 2 — credential-aware capability surface (host runtime). - New `VisibleCapabilityAccess::NeedsAuth`: an authorized+installed capability whose required credentials are missing is downgraded on the visible surface, for generic secret-handle credentials and product-auth (OAuth) accounts alike. - Presence is resolved through a new dependency-inverted `CapabilityCredentialPresence` port; the production impl composes `secret_present` and the new side-effect-free `RuntimeCredentialAccountResolver::account_configured` (select-only — the existing `resolve_access_secret` refreshes OAuth tokens and must never run on surface render) behind a short-TTL, owner-scoped, conclusive-only `CredentialPresenceCache` whose product-auth key includes sorted provider scopes (gmail readonly/send/modify must not alias). - Surface fingerprint stays credential-independent by construction: `surface_version` is computed from authorization-only access before the downgrade pass (see #4789), regression-pinned. - Fail-open discipline throughout: backend errors and missing wiring are indeterminate, never "missing" — a blip cannot burn a false sign-in prompt. Phase 3 — carry the state to the model. - `describe_with_access` folds a fixed, host-authored "[Not connected: ...]" sentence into the model-visible tool description at the single host->loop mapping seam, so both the provider tool definition and the descriptor view read the same folded string. Regression tests (all red-first): cross-credential-type group scenario (gmail Google OAuth / github GitHub OAuth / notion Notion OAuth+MCP must not render the ready message when Enabled-but-uncredentialed; credential-free web-access control still must), caller-level NeedsAuth contract tests through `HostRuntimeServices`, a panic-guard pinning that the surface path never calls the refreshing resolver, a cache scope-aliasing regression, the fingerprint-stability pin, and an e2e assertion that the captured model-visible tool definition carries the not-connected marker. Plan docs: docs/plans/2026-07-01-reborn-google-connection-state-5416.md (interim) and docs/plans/2026-07-01-reborn-tool-lifecycle-unified-pipeline-northstar.md (north star; phases 1/3+/4 tracked there). Closes #5416 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
scope: docs
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds credential-presence checks into runtime capability rendering, then switches extension search and onboarding to an availability-based projection with ChangesCredential presence and capability surface
Availability projection and search
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Poem Cache says: present, absent, or unsure; 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements Phase 2 and Phase 3 of the reborn connection-state unification plan (issue #5416). It introduces a short-TTL CredentialPresenceCache and a side-effect-free account_configured check to verify credential presence during capability-surface rendering without triggering expensive token refreshes. It also collapses model-facing connection states into a single ExtensionAvailability enum and appends a fixed not-connected marker to model-visible capability descriptions when required credentials are missing. The review feedback suggests deduplicating provider scopes after sorting them in ProductionCredentialPresence to ensure cache keys are canonical and independent of redundant entries.
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.
| let mut scopes = requirement.provider_scopes.clone(); | ||
| scopes.sort(); |
There was a problem hiding this comment.
To ensure the cache key is truly canonical and independent of duplicate scopes that might be declared in different orders or with redundant entries, consider deduplicating the scopes after sorting them.
let mut scopes = requirement.provider_scopes.clone();
scopes.sort();
scopes.dedup();There was a problem hiding this comment.
Pull request overview
This PR fixes Reborn’s contradictory “already connected” messaging by making extension/capability connection state credential-aware across (1) extension search results, (2) host runtime capability surfaces, and (3) the model-visible tool descriptions.
Changes:
- Replace model-facing
installation_phasewith a collapsedExtensionAvailability(available/needs_auth/not_installed/unknown) and gate the “ready” message + onboarding suppression on that projection. - Add credential-aware capability surfacing via
VisibleCapabilityAccess::NeedsAuth, backed by a side-effect-free product-auth presence check (account_configured) and a short-TTL, owner-scoped presence cache (including provider-scope keying). - Propagate
NeedsAuthto the model by folding a fixed “[Not connected…]” sentence into tool descriptions at the host→loop mapping seam; expand harness/test seams to assert against shipped tool definitions.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/support/reborn/harness.rs | Expose extension installation store seam for tests; add account_configured to the fixed resolver. |
| tests/support/reborn/group.rs | Retain TraceLlm handle for later inspection of shipped tool definitions. |
| tests/support/reborn/builder.rs | Add captured_tool_definitions() to integration harness via TraceLlm. |
| tests/reborn_integration_auth_gate.rs | Assert the model-visible tool description includes the NeedsAuth marker. |
| tests/reborn_group_extensions/scenario_search_ready_message_requires_credential.rs | New regression scenario: search “ready” message must require credentials across multiple providers + control case. |
| tests/reborn_group_extensions/scenario_remove_then_absent_cross_thread.rs | Update cross-thread assertions to use availability instead of installation_phase. |
| tests/reborn_group_extensions/scenario_install_then_visible_cross_thread.rs | Update cross-thread install visibility assertions to availability. |
| tests/reborn_group_extensions/scenario_activate_then_active_cross_thread.rs | Update activation persistence assertions; add durable-store read to prove activation specifically. |
| tests/reborn_group_extensions/main.rs | Register the new #5416 regression scenario and update scenario descriptions for availability. |
| docs/plans/2026-07-01-reborn-tool-lifecycle-unified-pipeline-northstar.md | Add north-star design doc describing unified connection-state pipeline. |
| docs/plans/2026-07-01-reborn-google-connection-state-5416.md | Add interim plan doc for fixing the Gmail connection-state contradiction. |
| crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rs | Implement side-effect-free account_configured for product-auth presence checks. |
| crates/ironclaw_reborn_composition/src/lib.rs | Register new extension_availability module. |
| crates/ironclaw_reborn_composition/src/extension_lifecycle.rs | Fan out search summaries with bounded concurrency; compute and apply availability; gate “ready” message via any_available. |
| crates/ironclaw_reborn_composition/src/extension_lifecycle_command.rs | Update test payload shape for availability field rename. |
| crates/ironclaw_reborn_composition/src/extension_lifecycle_capabilities.rs | Update local-dev tests to assert onboarding/message behavior via availability. |
| crates/ironclaw_reborn_composition/src/extension_availability.rs | New module: project (installed × credential readiness) → ExtensionAvailability and “any available?” predicate. |
| crates/ironclaw_product_workflow/tests/reborn_services_contract.rs | Document/validate Unknown credential readiness behavior in contract test. |
| crates/ironclaw_product_workflow/src/reborn_services/types.rs | Add ReverifyRequired onboarding state for unknown credential readiness. |
| crates/ironclaw_product_workflow/src/reborn_services/extensions.rs | Fail closed on Unknown readiness; keep Unknown actionable (needs_setup=true); update tests. |
| crates/ironclaw_product_workflow/src/reborn_services/extension_onboarding.rs | Add Unknown→reverify onboarding for plausibly-connected phases; add regression test. |
| crates/ironclaw_product_workflow/src/lifecycle.rs | Add ExtensionAvailability enum and replace installation_phase with availability on search summaries. |
| crates/ironclaw_product_workflow/src/lib.rs | Re-export ExtensionAvailability. |
| crates/ironclaw_loop_support/src/lib.rs | Re-export CAPABILITY_NEEDS_AUTH_DESCRIPTION_MARKER. |
| crates/ironclaw_loop_support/src/capability_port.rs | Fold NeedsAuth marker into model-visible descriptions and descriptor views; add unit+integration coverage. |
| crates/ironclaw_host_runtime/tests/runtime_http_egress_contract.rs | Implement new account_configured method on test resolver. |
| crates/ironclaw_host_runtime/tests/host_runtime_visible_capability_credential_presence_contract.rs | New caller-level contract tests for NeedsAuth downgrade + side-effect guard. |
| crates/ironclaw_host_runtime/tests/github_wasm_runtime_contract.rs | Add account_configured to test resolvers. |
| crates/ironclaw_host_runtime/tests/builtin_obligation_handler_contract.rs | Add account_configured to test resolvers. |
| crates/ironclaw_host_runtime/src/surface.rs | Add VisibleCapabilityAccess::NeedsAuth and credential-presence port; keep fingerprint stable by downgrading post-hash. |
| crates/ironclaw_host_runtime/src/services.rs | Wire runtime credential resolver into host runtime for surface presence checks. |
| crates/ironclaw_host_runtime/src/production.rs | Add resolver+cache fields; build/wire ProductionCredentialPresence into visible_capabilities. |
| crates/ironclaw_host_runtime/src/obligations.rs | Extend resolver trait with side-effect-free account_configured. |
| crates/ironclaw_host_runtime/src/lib.rs | Register new credential presence modules. |
| crates/ironclaw_host_runtime/src/credential_presence.rs | New production credential presence implementation (secret store + account resolver + cache). |
| crates/ironclaw_host_runtime/src/credential_presence_cache.rs | New short-TTL, owner-scoped presence cache with scope-aware keys (incl. provider scopes). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let create_issue_tool = first_call_tools | ||
| .iter() | ||
| .find(|tool| tool.name.contains("create_issue")) | ||
| .expect("github.create_issue is on the model-visible tool list"); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68cdb4702d
ℹ️ 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".
| let key = CredentialPresenceKey::ProductAuth( | ||
| owner_scope.clone(), | ||
| requirement.provider.clone(), | ||
| requirement.requester_extension.clone(), | ||
| scopes, |
There was a problem hiding this comment.
When an extension declares two product-auth requirements with the same owner/provider/requester/scopes but different setup modes, this cache key aliases their answers even though account_configured is called with requirement.setup and selection behavior depends on it (for example ManualToken ignores stored scopes while OAuth requires them). The first rendered requirement can therefore cache true/false and make the other capability show the wrong Available/NeedsAuth state for the TTL; include the setup in the key.
Useful? React with 👍 / 👎.
| Ok(account) => Ok(account.status == CredentialAccountStatus::Configured | ||
| && account.access_secret.is_some()), |
There was a problem hiding this comment.
If the account store returns a Configured account with no access_secret, the dispatch path classifies that as Backend corruption rather than a user auth problem, but this presence check returns Ok(false). That false result is cached and downgrades the model surface to NeedsAuth, causing a re-auth prompt for a backend/data-integrity issue that re-auth may not fix; mirror resolve_access_secret/runtime_credential_auth_requirement_configured and return Err(CredentialStageError::Backend) when the selected account lacks an access secret.
Useful? React with 👍 / 👎.
|
🚅 Deployed to the ironclaw-pr-5528 environment in ironclaw-ci-preview
|
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Fix Reborn extension connection state so uncredentialed extensions are shown as needing auth across search, capability surface, and model tool descriptions.
Stats: 6 net-new findings (from 10 raw, 6 after live-thread duplicate suppression) across 4 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0. Suppressed as already covered by live unresolved threads: 3.
Existing live threads not duplicated: product-auth cache key omits setup mode, configured account without access_secret should be backend corruption, and duplicate scope canonicalization are already open on this head.
performance
- Medium Credential presence checks serialize storage I/O per tool render (
crates/ironclaw_host_runtime/src/surface.rs:252-260, confidence 78) — anchor: crates/ironclaw_host_runtime/src/surface.rs:252
visible_capabilitiesnow runs on every model surface/tool render, but the new downgrade pass awaitsrequired_credentials_presentone capability at a time. On a cold cache or after the 10s TTL, each credentialed capability can perform secret-store or product-auth account lookups, so a surface with N credentialed tools adds N serialized backend round trips before the model can see tools. The cache only helps after the first completed pass and does not prevent the first render after expiry from paying the full serial latency.
tests
- Medium Secret-store backend errors lack fail-open coverage (
crates/ironclaw_host_runtime/src/credential_presence.rs:63-74, confidence 75) — anchor: crates/ironclaw_host_runtime/src/credential_presence.rs:63
The new secret-handle presence path treatssecret_presenterrors as indeterminate, but the caller-level tests cover product-auth backend failures and dispatch-time secret failures without covering this surface-render path. A regression could downgrade a capability toNeedsAuthon secret-store metadata failure despite the stated fail-open contract. - Low Owner-scope projection lacks ResourceScope churn test (
crates/ironclaw_host_runtime/src/credential_presence_cache.rs:56-62, confidence 75) — anchor: crates/ironclaw_host_runtime/src/credential_presence_cache.rs:56
The cache relies onCredentialOwnerScope::from_scopedropping invocation/thread/mission ids so presence survives per-renderResourceScopechurn, but existing cache tests build owner scopes manually and never exercise this projection from realResourceScopevalues.
local-patterns
- Low Cache docs describe the old product-auth presence contract (
crates/ironclaw_host_runtime/src/credential_presence_cache.rs:10-12, confidence 75) — anchor: crates/ironclaw_host_runtime/src/credential_presence_cache.rs:10
The module docs still say product-auth conclusive results areAuthRequired-vs-resolved, but the new local contract isaccount_configuredreturningOk(true)/Ok(false)and treating backend failures as indeterminate. That stale wording points future maintainers back toward the dispatch-time resolver semantics this PR is trying to avoid. - Low Failure text points at a deleted lifecycle helper (
tests/reborn_group_extensions/scenario_search_ready_message_requires_credential.rs:107-114, confidence 75) — anchor: tests/reborn_group_extensions/scenario_search_ready_message_requires_credential.rs:107
The regression failure still sayssearch_installation_phasemust consult the credential gate forLifecyclePhase::Active, but this PR removed that search-summary shape and now routes through the availability projection. If this test fails later, the message sends the maintainer to a deleted symbol instead of the current credential-gated availability path.
maintainability
- Low Do not encode credential presence as Option (
crates/ironclaw_host_runtime/src/surface.rs:117-126, confidence 75) — anchor: crates/ironclaw_host_runtime/src/surface.rs:117
The new credential-presence port exposes a three-state contract asOption<bool>: present, missing, and indeterminate/fail-open. The caller has to rely on== Some(false)to preserve fail-open behavior, which hides the core invariant at the boundary betweensurface.rsandcredential_presence.rs.
| &capabilities, | ||
| )?; | ||
|
|
||
| if let Some(presence) = self.credential_presence { |
There was a problem hiding this comment.
Medium — Credential presence checks serialize storage I/O per tool render.
visible_capabilities now runs on every model surface/tool render, but the new downgrade pass awaits required_credentials_present one capability at a time. On a cold cache or after the 10s TTL, each credentialed capability can perform secret-store or product-auth account lookups, so a surface with N credentialed tools adds N serialized backend round trips before the model can see tools. The cache only helps after the first completed pass and does not prevent the first render after expiry from paying the full serial latency.
Fix: Collect available capabilities needing credential checks, deduplicate equivalent requirements, and resolve them with bounded concurrency before applying the NeedsAuth downgrades.
| any_missing |= !present; | ||
| continue; | ||
| } | ||
| match secret_present(store, scope, handle).await { |
There was a problem hiding this comment.
Medium — Secret-store backend errors lack fail-open coverage.
The new secret-handle presence path treats secret_present errors as indeterminate, but the caller-level tests cover product-auth backend failures and dispatch-time secret failures without covering this surface-render path. A regression could downgrade a capability to NeedsAuth on secret-store metadata failure despite the stated fail-open contract.
Fix: Add caller-level coverage for ProductionCredentialPresence/visible capability rendering where SecretStore::metadata errors return indeterminate/Available and do not cache a false missing credential.
| //! far less often than that — so the naive approach would issue N redundant | ||
| //! presence lookups per step for no benefit. | ||
| //! | ||
| //! Only CONCLUSIVE presence results are cached (`Ok(true)`/`Ok(false)` from the |
There was a problem hiding this comment.
Low — Cache docs describe the old product-auth presence contract.
The module docs still say product-auth conclusive results are AuthRequired-vs-resolved, but the new local contract is account_configured returning Ok(true)/Ok(false) and treating backend failures as indeterminate. That stale wording points future maintainers back toward the dispatch-time resolver semantics this PR is trying to avoid.
Fix: Update the sentence to describe the side-effect-free account_configured result shape, for example: Ok(true)/Ok(false) from the product-auth account resolver; backend errors are never cached.
| } | ||
|
|
||
| impl CredentialOwnerScope { | ||
| pub(crate) fn from_scope(scope: &ResourceScope) -> Self { |
There was a problem hiding this comment.
Low — Owner-scope projection lacks ResourceScope churn test.
The cache relies on CredentialOwnerScope::from_scope dropping invocation/thread/mission ids so presence survives per-render ResourceScope churn, but existing cache tests build owner scopes manually and never exercise this projection from real ResourceScope values.
Fix: Add a cache test that builds two ResourceScopes with the same owner axes but different invocation/thread/mission ids and verifies CredentialOwnerScope::from_scope produces the same cache key.
| /// `RuntimeCredentialAccountResolver` behind a short-TTL cache. | ||
| #[async_trait] | ||
| pub(crate) trait CapabilityCredentialPresence: Send + Sync { | ||
| /// `Some(true)` = all required credentials present (or none required); |
There was a problem hiding this comment.
Low — Do not encode credential presence as Option.
The new credential-presence port exposes a three-state contract as Option<bool>: present, missing, and indeterminate/fail-open. The caller has to rely on == Some(false) to preserve fail-open behavior, which hides the core invariant at the boundary between surface.rs and credential_presence.rs.
Fix: Replace the Option<bool> return with a small crate-local enum such as CredentialPresenceStatus::{Present, Missing, Indeterminate} and have visible_capabilities match only Missing for the NeedsAuth downgrade.
|
|
||
| if !bugs.is_empty() { | ||
| return Err(format!( | ||
| "bug #5416: `builtin.extension_search` mis-reported credential readiness for: {bugs:?}. \ |
There was a problem hiding this comment.
Low — Failure text points at a deleted lifecycle helper.
The regression failure still says search_installation_phase must consult the credential gate for LifecyclePhase::Active, but this PR removed that search-summary shape and now routes through the availability projection. If this test fails later, the message sends the maintainer to a deleted symbol instead of the current credential-gated availability path.
Fix: Rewrite the failure text around availability/resolve_extension_availability or the credential-gated projection, rather than search_installation_phase and LifecyclePhase::Active.
…ption classification, concurrent presence resolution (#5416) Review-comment fixes on the connection-state PR: - Cache key: ProductAuth entries now also key on the credential setup kind (ManualToken vs OAuth select differently) and provider scopes are sorted AND deduplicated, so equivalent declarations share one canonical key and differing requirements never alias. - account_configured: a Configured account with no access_secret is data corruption, not a missing credential — classify it Backend (indeterminate, uncached, capability stays Available) mirroring resolve_access_secret, instead of Ok(false) which rendered a NeedsAuth re-auth prompt that re-auth cannot fix. - Surface downgrade pass resolves credential presence with bounded concurrency (buffered(8), order-preserving positional apply) instead of N serialized backend round trips per cold render — mirrors the search path's EXTENSION_READINESS_CONCURRENCY pattern. - Presence port returns a three-state CredentialPresenceStatus (Present/Missing/Indeterminate) instead of Option<bool>; the downgrade matches only Missing, making the fail-open invariant a type-level contract. - Tests: fail-open coverage for secret-store metadata errors on the render path (toggleable failing store, asserts no stale cache entry); owner-scope projection churn test from real ResourceScopes; setup-kind key-distinctness; 20-capability concurrent downgrade ordering regression; exact tool-name assertion (github__create_issue, no create_issue_comment collision). - Docs: cache module contract rewritten to the side-effect-free account_configured shape; group-scenario failure text no longer references deleted search_installation_phase symbols. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All review comments addressed in 6206ee9 — thanks, every one was valid:
Verification: host_runtime 307 lib + all contracts green (3 pre-existing docker-socket sandbox_process failures only, reproduced on the base commit via stash), composition product_auth 120/120, both e2e suites 22/22, clippy byte-identical to base (stash-diffed), fmt clean. 🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/plans/2026-07-01-reborn-google-connection-state-5416.md`:
- Around line 189-231: The design doc contains duplicated near-verbatim
paragraphs for the “Concurrency (round-2 finding B)” and “Behavior-flip to lock
with a test” sections, likely from a merge artifact. Collapse the repeated text
into a single authoritative copy, keeping one version of each point in the
discussion around search readiness and the
`search()`/`lifecycle_extension_infos` behavior so the plan reads cleanly and
unambiguously.
🪄 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: 761b86ff-5c8f-454d-9988-46e9ef1a5141
📒 Files selected for processing (37)
crates/ironclaw_host_runtime/src/credential_presence.rscrates/ironclaw_host_runtime/src/credential_presence_cache.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/src/obligations.rscrates/ironclaw_host_runtime/src/production.rscrates/ironclaw_host_runtime/src/services.rscrates/ironclaw_host_runtime/src/surface.rscrates/ironclaw_host_runtime/tests/builtin_obligation_handler_contract.rscrates/ironclaw_host_runtime/tests/github_wasm_runtime_contract.rscrates/ironclaw_host_runtime/tests/host_runtime_visible_capability_credential_presence_contract.rscrates/ironclaw_host_runtime/tests/runtime_http_egress_contract.rscrates/ironclaw_loop_support/src/capability_port.rscrates/ironclaw_loop_support/src/lib.rscrates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/src/lifecycle.rscrates/ironclaw_product_workflow/src/reborn_services/extension_onboarding.rscrates/ironclaw_product_workflow/src/reborn_services/extensions.rscrates/ironclaw_product_workflow/src/reborn_services/types.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_reborn_composition/src/extension_availability.rscrates/ironclaw_reborn_composition/src/extension_lifecycle.rscrates/ironclaw_reborn_composition/src/extension_lifecycle_capabilities.rscrates/ironclaw_reborn_composition/src/extension_lifecycle_command.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rscrates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests.rsdocs/plans/2026-07-01-reborn-google-connection-state-5416.mddocs/plans/2026-07-01-reborn-tool-lifecycle-unified-pipeline-northstar.mdtests/reborn_group_extensions/main.rstests/reborn_group_extensions/scenario_activate_then_active_cross_thread.rstests/reborn_group_extensions/scenario_install_then_visible_cross_thread.rstests/reborn_group_extensions/scenario_remove_then_absent_cross_thread.rstests/reborn_group_extensions/scenario_search_ready_message_requires_credential.rstests/reborn_integration_auth_gate.rstests/support/reborn/builder.rstests/support/reborn/group.rstests/support/reborn/harness.rs
| **Concurrency (round-2 finding B).** Today the sequential per-extension credential | ||
| check in `search()` (`extension_lifecycle.rs:194` — `for extension { push(search_ | ||
| summary(..).await) }`) only fires for `Installed`-phase extensions (A1 skips | ||
| `Active`). Projecting `availability` for **every** result widens a live | ||
| O(extensions × requirements) sequential await chain (each `missing_requirements` | ||
| hits account selection per requirement). The list path already solved this one file | ||
| away: `lifecycle_extension_infos` (`reborn_services/extensions.rs:192-213`) fans out | ||
| with `stream::iter(..).buffered(EXTENSION_READINESS_CONCURRENCY)` (const = 8, | ||
| ceiling test at `:515`; `futures = "0.3"` is already a composition dep, | ||
| `Cargo.toml:111`). Adopt the same pattern for the search loop. Lives in the | ||
| extracted module (§4.6). | ||
|
|
||
| **Backend degrade.** A `Backend` blip currently **fails the entire search** | ||
| (`map_search_credential_stage_error` → `Transient`). New: it becomes `unknown` | ||
| (search still returns; that extension shown not-`available`). Fail-closed for the | ||
| "ready" claim, search stays up. | ||
|
|
||
| **Behavior-flip to lock with a test (round-1 finding #7):** the old | ||
| `search_credentials_configured` short-circuited `requirements.is_empty() → false` | ||
| *before* consulting the gate, so a **credential-less** extension never reached | ||
| "ready" via search. Under the projection it becomes `available` for an installed | ||
| credential-less extension. That is the intended, more-correct | ||
|
|
||
| **Concurrency (round-2 finding B).** Today the sequential per-extension credential | ||
| check in `search()` (`extension_lifecycle.rs:194` — `for extension { push(search_ | ||
| summary(..).await) }`) only fires for `Installed`-phase extensions (A1 skips | ||
| `Active`). Closing A1 means computing readiness for **every** result, widening a | ||
| live O(extensions × requirements) sequential await chain (each | ||
| `missing_requirements` hits account selection per requirement). The list path | ||
| already solved this one file away: `lifecycle_extension_infos` | ||
| (`reborn_services/extensions.rs:192-213`) fans out with | ||
| `stream::iter(..).buffered(EXTENSION_READINESS_CONCURRENCY)` (const = 8, with a | ||
| ceiling test at `:515`). Adopt the **same** canonical pattern for the search loop — | ||
| don't leave a widened sequential fan-out. This lives naturally in the extracted | ||
| module (§4.6). | ||
|
|
||
| **Behavior-flip to lock with a test (round-1 finding #7):** the old | ||
| `search_credentials_configured` short-circuited `requirements.is_empty() → false` | ||
| *before* consulting the gate, so a **credential-less** extension never reached | ||
| "ready" via search. Under the projection it becomes `available` for an installed | ||
| credential-less extension. That is the intended, more-correct behavior (an | ||
| extension needing no credentials is legitimately "don't ask for credentials"), but | ||
| it is a silent change — pin it explicitly in §6. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Duplicated paragraph in the design doc.
Lines 189-199 and 212-223 ("Concurrency (round-2 finding B)") and lines 206-211 and 225-231 ("Behavior-flip to lock with a test") are near-verbatim duplicates — looks like a merge artifact from combining review rounds. Worth collapsing to one copy so future readers don't wonder which is authoritative.
🤖 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 `@docs/plans/2026-07-01-reborn-google-connection-state-5416.md` around lines
189 - 231, The design doc contains duplicated near-verbatim paragraphs for the
“Concurrency (round-2 finding B)” and “Behavior-flip to lock with a test”
sections, likely from a merge artifact. Collapse the repeated text into a single
authoritative copy, keeping one version of each point in the discussion around
search readiness and the `search()`/`lifecycle_extension_infos` behavior so the
plan reads cleanly and unambiguously.
|
Closing as stale — no activity in over three weeks. The branch is untouched; reopen if this is still needed. |
Fixes #5416 — the Reborn agent claimed Gmail (or any credentialed extension) was "already configured or active — do not ask the user for credentials" while no credential account existed, then contradicted itself with an auth gate on first use. Third patch on this oscillating predicate (#4996 → #5037 → here); this one replaces the predicate class instead of re-tuning it.
Root cause
extension_search'ssearch_installation_phaseread the lifecycle axis only: anEnabled-but-uncredentialed installation short-circuited to Active without ever consulting the credential gate, andsuppress_search_credential_onboardingblanket-cleared the credential hints — so the model-visible "ready, don't ask for credentials" message fired on a disconnected extension.What this does (three shippable slices, one design)
Phase 0 — collapsed model-facing availability (search surface).
One
ExtensionAvailabilitystate (available/needs_auth/not_installed/unknown) projected from installed? × credential-readiness, replacing the leaked lifecycleinstallation_phaseon search summaries. The blanket suppression helpers are deleted; the ready message requires a genuinelyavailableprojection, and how-to-connect detail stays on everything else. Settingsextension_infonow fails closed onUnknownreadiness (Defect B) with a phase-gatedReverifyRequiredonboarding state.Phase 2 — credential-aware capability surface (host runtime).
New
VisibleCapabilityAccess::NeedsAuth: an authorized+installed capability whose required credentials are missing is downgraded on the visible surface — generic secret-handle credentials and product-auth (OAuth) accounts alike, via a dependency-invertedCapabilityCredentialPresenceport. Highlights:RuntimeCredentialAccountResolver::account_configured(select-only). The existingresolve_access_secretrefreshes OAuth tokens and must never run on surface render — pinned by a panic-guard regression test.CredentialPresenceCache; the product-auth key includes sorted provider scopes so gmail readonly/send/modify cannot alias (regression-tested).surface_versionis computed from authorization-only access before the downgrade pass, so credential flips never churn the surface version ([Reborn] Capability execution fails withdriver_unavailableafter first successful tool call #4789 discipline, regression-pinned).Phase 3 — carry the state to the model.
describe_with_accessfolds a fixed, host-authored[Not connected: …]sentence into the model-visible tool description at the single host→loop mapping seam (feeds both the provider tool definition and the descriptor view from one binding).Tests (all written red-first)
Enabledwith no credential account must NOT render the ready message; credential-free web-access control still must (over-suppression guard).NeedsAuthcontract tests throughHostRuntimeServices(generic secret absent, product-authAuthRequired, backend-error fail-open).github.create_issuecarries the not-connected marker through the production wiring.Review process
Implemented per docs/plans/2026-07-01-reborn-google-connection-state-5416.md (interim) + docs/plans/2026-07-01-reborn-tool-lifecycle-unified-pipeline-northstar.md (north star — phases 1/4 and the structured-schema variant of phase 3 tracked there). Three adversarial review rounds; two blockers found and fixed red-first (OAuth-refresh-on-render, scope-aliased cache key).
Verification
ironclaw_host_runtime: 303 lib + all contract targets green (3 pre-existing docker-socketsandbox_processfailures only, untouched)ironclaw_product_workflow: 201 contract + 87 lib greenironclaw_reborn_composition: product_auth suites greenironclaw_loop_support: all targets greenreborn_group_extensions+reborn_integration_auth_gate: 22/22 eachcargo fmt --checkclean🤖 Generated with Claude Code