feat(reborn): Slack personal OAuth foundations — dormant additive layer (stack 2/4) - #5644
Conversation
Additive layer for Slack personal OAuth; no user-visible change. The catalog still offers pairing, all pairing endpoints remain intact, and the new OAuth surface stays dormant until the serve/webui wiring lands in the next PR of this stack. - ironclaw_auth: OAuth primitives generalized beyond Google (OAuthCallbackState newtype, OAuthRedirectUri), provider_identity on credential accounts, SLACK_PERSONAL constants, owner-granularity cleanup + provider selector, contract tests. - oauth_provider_client: SlackAuthedUser parsing, expires_in=0 (non-expiring token) fix, exchange logging. - host_api http + host_runtime egress: execute_credential_exchange with fail-closed enforcement + runtime egress contract tests. - oauth_gate: unified OAuth gate driver (Google refactored onto it, OAuthGateProviderRegistry) incl. fallthrough fix; ripple into google_oauth/notion_oauth/oauth_dcr/nearai_mcp. - slack_personal_oauth (new) + product_auth_serve: Slack OAuth start/callback routes, identity hook, failure-HTML signal. - slack_setup: oauth client-id/secret slot + personal_oauth_ready; slack_channel_connection; SlackPersonalUserBinder trait; product_auth_durable cleanup/flows. - slack_user tool: full WASM user-token tool (tools-src + first_party_extensions assets incl. committed wasm). - gsuite account_policy ctor ripple; provider_identity field ripple across struct literals. Transitional #[allow(dead_code)] markers sit on four pub(crate) hooks that the serve/slack_host_beta wiring consumes in the next stack PR; they disappear when those files reach their final reviewed state. Mechanical re-slice (2/4) of the fully-reviewed #5604 head 178829a. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Slack personal OAuth, provider identity propagation, credential-exchange egress bypass, owner/provider-granularity cleanup, Slack setup OAuth credentials, channel disconnect revocation, and a new ChangesCore Auth Contract
Estimated code review effort: 5 (Critical) | ~180 minutes Provider-Agnostic OAuth and Slack Personal Flow
Slack User WASM Tool
Sequence Diagram(s)sequenceDiagram
participant User
participant ProductAuthServe
participant Registry
participant SlackAPI
participant AuthServices
participant Binder
User->>ProductAuthServe: start_slack_personal_oauth_flow
ProductAuthServe->>Registry: prepare_flow
Registry-->>ProductAuthServe: auth URL + PKCE/state
User->>ProductAuthServe: callback(code, state)
ProductAuthServe->>SlackAPI: execute_credential_exchange
SlackAPI-->>ProductAuthServe: token + provider_identity
ProductAuthServe->>AuthServices: handle_oauth_callback
AuthServices->>Binder: bind_personal_user
ChangesSlack personal OAuth and provider wiring
Slack User WASM Tool
Estimated code review effort: 5 (Critical) | ~180 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces the slack_user first-party extension, enabling IronClaw to act on behalf of users in Slack via personal user tokens (xoxp-). It unifies the OAuth gate flow driver and callback state handling to support both Google and Slack personal providers, adds a dedicated unsanitized egress path (execute_credential_exchange) for secure host token exchanges, and implements per-user disconnect cleanup. Feedback is provided regarding a security risk in slack_setup.rs where a client secret is unnecessarily exposed in memory, creating a plain-text heap allocation, which can be avoided by directly using the returned SecretString.
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.
|
🚅 Deployed to the ironclaw-pr-5644 environment in ironclaw-ci-preview
|
…2-slack-oauth-foundations
|
Base-sync note: after Correctness proof of the whole sync: I built a reference merge of the reviewed #5604 head 🤖 Generated with Claude Code |
serrrfirat
left a comment
There was a problem hiding this comment.
Multi-agent review completed for the Slack personal OAuth foundations slice.
Reviewed with security, bugs, performance/concurrency, tests, and conventions lenses. I found 6 actionable Medium issues after deduping overlapping findings. The strongest risks are partial-failure behavior around Slack setup/callback, a staged network-policy leak on successful OAuth exchanges, and a couple of dormant-but-important Slack user-token safety issues before PR 3 exposes the surface.
No Critical/High findings, so posting as COMMENT rather than REQUEST_CHANGES per the review policy.
Review follow-ups from the multi-agent + Gemini review of the foundations slice: - slack_setup: return the client secret SecretString directly instead of expose->to_string->rewrap (SecretMaterial is a SecretString alias; drops a needless plaintext heap copy). - slack_setup: rollback_failed_activation_save now also deletes the failed save's fresh oauth_client_secret_handle (guarded by the same previous/protected-current reference check as bot/signing handles); extended the rollback test and added an inherited-handle regression. - ironclaw_auth: OAuthProviderIdentitySubject deserializes through validation (#[serde(try_from = "String")] + TryFrom delegating to new) per the types.md newtype rule; the type is new in this stack so no persisted rows predate the tightening. - oauth_provider_client + oauth_dcr: consume the staged network policy after every credential exchange (success or failure) via the obligation handler's abort seam; the egress pipeline only discards on pre-transport errors, so successful exchanges leaked one unbounded policy-store entry per flow. Regression asserts the staged policy is discarded after a successful exchange. - product_auth_serve: the Slack callback identity hook now returns a compensating rollback; if complete_oauth_callback fails after the hook durably bound the Slack identity, the completion-failure arm deletes exactly that binding (scoped by full provider_user_id) so a failed completion cannot leave Slack "connected" with no usable credential. Threaded a RebornUserIdentityBindingDeleteStore through SlackPersonalOAuthBindingConfig; regression drives the route-level harness with a failing completion and asserts bind-then-rollback. - slack_user tool: log only the static Slack API resource name (strip the query string carrying search terms / channel ids / timestamps) in both source copies; committed wasm rebuilt with cargo-component (wasm32-wasip2). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up from #5604 (Gemini): the two slack parse-failure tests asserted only is_err(), so an unrelated parse failure could keep them green. Both now assert the specific AuthErrorCode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
compose_provider_client_with_runtime takes a `#[cfg(feature = "slack-v2-host-beta")]`-gated 5th parameter (slack_personal_oauth_slot). The production wrapper gates the argument it forwards with the same cfg, but the four `#[tokio::test]` call sites in product_auth_providers.rs passed `None` unconditionally. Without the feature the function takes 4 args, so the tests supply one too many and fail to compile (E0061) — breaking `cargo test` and `cargo clippy --tests` in any default-feature build of this crate. Gate each test call site's 5th argument exactly like the production wrapper. The composition lib-tests now compile under both the default and the slack-v2-host-beta feature sets. Why CI stayed green until now: code_style.yml runs the SLIM clippy matrix (--all-features only) for pull_request and merge_group, where the feature is on and the tests compile. Only the FULL matrix on push-to-main adds the no-features `default` leg that trips E0061 — so this latent break would have turned main red after the Slack stack merged. That default leg is the regression guard for this fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
⏳ IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted review state before this projection. |
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Verdict: ❌ Changes requested
Findings: 2 blocking / 0 notes
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Head: dde131a23b9ed4e88d39ff56a8b47409390c50a1
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found blocking integration issues in the new Slack personal extension path: the bundled slack_user package is not exposed/materialized by the available-extension catalog, and its OAuth start path rejects the slack_user package id even though the credential is declared by that package.
Findings
1. ❌ [HIGH] Bundled slack_user extension is omitted from the available-extension catalog
Location: crates/ironclaw_reborn_composition/src/available_extensions.rs:358
The new slack_user manifest and WASM assets are added and the trust policy references /system/extensions/slack_user/manifest.toml, but from_first_party_assets_with_nearai_mcp_config only pushes the bot slack_package() under slack-v2-host-beta. There is no slack_user_package() or asset list, so local-dev/first-party catalog construction never exposes or materializes the new package. Users cannot install/activate the Slack personal tools from the bundled catalog, and the trust-policy entry points at a manifest path that the catalog does not write.
2. ❌ [HIGH] Slack personal OAuth start rejects the slack_user package id
Location: crates/ironclaw_reborn_composition/src/product_auth_serve/oauth.rs:208
The Slack personal OAuth start handler requires requester_extension to equal SLACK_EXTENSION_ID (slack), but the new tool package and its runtime credential source are declared under the slack_user extension id. The extension OAuth route is scoped by the package being activated, so once slack_user is available its OAuth start request will be rejected with invalid_request; if routed through slack instead, the created credential binding is for the wrong owner extension and the slack_user.* capabilities will not be able to use it. This should authorize/bind the slack_user extension id for the slack_personal provider.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
Inline review fallback
Inline comment projection fell back to a body-only PR Review because GitHub rejected the inline payload.
Reason: Unprocessable Entity: "Line could not be resolved" - https://docs.github.com/rest/pulls/reviews#create-a-review-for-a-pull-request
IronLoop preserved the inline review comment payloads below instead of dropping them.
Inline fallback 1: crates/ironclaw_reborn_composition/src/available_extensions.rs:358
This only adds the bot Slack package to the first-party catalog. The PR adds assets/slack_user/* and a trust-policy entry for /system/extensions/slack_user/manifest.toml, but without a slack_user_package() plus assets here, the bundled catalog never exposes or materializes the new Slack personal extension.
Inline fallback 2: crates/ironclaw_reborn_composition/src/product_auth_serve/oauth.rs:208
This check appears to use the bot Slack package id (slack) for a credential declared by the new slack_user extension. The extension OAuth route is package-scoped, so activating slack_user will hit this branch with requester_extension == "slack_user" and get rejected; routing through slack would also bind the account to the wrong owner extension for slack_user.* capabilities.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/factory.rs (1)
3661-3695: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winGate the slack slot discard behind the no-storage cfg
crates/ironclaw_reborn_composition/src/factory.rs:3679-3695— the standalone#[cfg(feature = "slack-v2-host-beta")] let _ = slack_personal_oauth_lazy_slot;drops the slot before thelibsql/postgresarms buildRebornProductionBuildContext, so any build that enablesslack-v2-host-betawith storage features hits a use-after-move.Fix
- #[cfg(feature = "slack-v2-host-beta")] + #[cfg(all(feature = "slack-v2-host-beta", not(any(feature = "libsql", feature = "postgres"))))] let _ = slack_personal_oauth_lazy_slot;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_composition/src/factory.rs` around lines 3661 - 3695, The standalone slack slot discard in factory setup is compiled even when storage features are enabled, which causes the value to be dropped too early before RebornProductionBuildContext is built. Move the slack_personal_oauth_lazy_slot discard so it is only applied in the same no-storage cfg path as the other unused inputs, and keep the libsql/postgres production_config path untouched. Use the existing cfg blocks around production_config and the no-storage let _ tuple in factory.rs to ensure slack-v2-host-beta does not trigger a use-after-move when storage is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_auth/src/oauth.rs`:
- Around line 604-612: OAuthProviderIdentitySubject only exposes as_str(), but
the validated-newtype boundary is missing the required as_ref() and into_inner()
methods. Update the impl for OAuthProviderIdentitySubject to add explicit
as_ref() returning a string reference and into_inner() consuming the wrapper to
return the inner String, matching the existing boundary style used by validated
newtypes.
In `@crates/ironclaw_auth/tests/auth_product_contract/cleanup_contract.rs`:
- Around line 408-496: The provider-isolation assertion in cleanup_for_lifecycle
is too weak because other_provider_account is skipped by ownership before
provider matching is exercised. Update the test setup in
cleanup_matches_owner_granularity_and_provider_selected_oauth_accounts so
other_provider_account is owned/granted like the slack extension-owned account,
then keep the provider: None uninstall cleanup assertion to prove that
ownership-matched but provider-mismatched accounts are not revoked. Reference
the existing cleanup_for_lifecycle, SecretCleanupRequest, and
other_provider_account setup to locate the change.
In `@crates/ironclaw_first_party_extensions/assets/slack_user/manifest.toml`:
- Line 17: The Slack manifest currently gives every capability the full provider
scope set, which is too broad for the per-capability contract. Update the
`provider_scopes` in `slack_user_token` so each capability like
`search_messages`, `list_conversations`, `get_conversation_history`, and
`get_user_info` only declares the scopes it actually needs, while leaving
`setup.scopes` broader if necessary. Keep the change localized to the manifest
entry and use the existing capability names to split the scope requirements
appropriately, including removing unnecessary `chat:write` where it is not
needed.
In
`@crates/ironclaw_first_party_extensions/assets/slack_user/wasm-src/src/types.rs`:
- Around line 25-34: The `sort` field in the Slack search request type is
currently modeled as `Option<String>`, which allows invalid values past the WASM
boundary and encourages string-literal checks. Update the request type in
`types.rs` to use a small enum for `sort` with `#[serde(rename_all =
"snake_case")]` and variants for `score` and `timestamp`, then keep the field as
`Option<ThatEnum>` so callers handle only valid values.
In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 3593-3603: The slack_user trust entry is currently reusing
gsuite_allowed_effects(), which couples Slack’s authority ceiling to the GSuite
helper. Add a dedicated slack_user_allowed_effects() near
gsuite_allowed_effects() with the intended Slack personal OAuth effect set, and
update AdminEntry::for_local_manifest in factory.rs to call that new helper
instead of the GSuite one.
In `@crates/ironclaw_reborn_composition/src/oauth_provider_client.rs`:
- Around line 955-994: The early return in discard_oauth_egress_policy is
swallowing oauth_execution_context failures without any observability. Replace
the Err(_) => return path with a warning log that records the context-building
failure before exiting, similar to the tracing::warn! used for the handler.abort
branch. Keep the cleanup best-effort behavior unchanged, but make sure the new
log includes enough context to identify the discard_oauth_egress_policy /
oauth_execution_context failure path.
In `@crates/ironclaw_reborn_composition/src/product_auth_durable/cleanup.rs`:
- Around line 21-45: The account cleanup path should continue using
owner-granularity lookup rather than exact scope matching, and the subsequent
read must use the account’s stored scope. In cleanup.rs, keep the
`CredentialAccountOwnerScope::from_scope` lookup and the filtering by extension
ownership/grant/provider in the `account_records_for_owner` loop, then ensure
`read_account` is called with `account.scope` plus `account.id` so
lifecycle/disconnect callers with fresh invocation IDs can find the stored
credential record consistently.
In `@crates/ironclaw_reborn_composition/tests/auth_callbacks.rs`:
- Around line 393-424: The host-binding callback test in
oauth_callback_handler_returns_provider_identity_for_host_binding misses
redacting enterprise_id from the serialized response. Update the anti-leak
assertion to also verify that the OAuthProviderIdentity field carrying "E123" is
absent from the JSON emitted by handle_oauth_callback, alongside the existing
checks for U123, T123, and A123.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 3661-3695: The standalone slack slot discard in factory setup is
compiled even when storage features are enabled, which causes the value to be
dropped too early before RebornProductionBuildContext is built. Move the
slack_personal_oauth_lazy_slot discard so it is only applied in the same
no-storage cfg path as the other unused inputs, and keep the libsql/postgres
production_config path untouched. Use the existing cfg blocks around
production_config and the no-storage let _ tuple in factory.rs to ensure
slack-v2-host-beta does not trigger a use-after-move when storage is enabled.
🪄 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: 0c9647c5-96b6-47e1-84cb-3b531df089fd
⛔ Files ignored due to path filters (1)
crates/ironclaw_first_party_extensions/assets/slack_user/wasm/slack_user_tool.wasmis excluded by!**/*.wasm,!**/*.wasm
📒 Files selected for processing (80)
crates/ironclaw_auth/src/cleanup.rscrates/ironclaw_auth/src/credential.rscrates/ironclaw_auth/src/domain.rscrates/ironclaw_auth/src/fakes.rscrates/ironclaw_auth/src/flow.rscrates/ironclaw_auth/src/lib.rscrates/ironclaw_auth/src/oauth.rscrates/ironclaw_auth/src/provider.rscrates/ironclaw_auth/tests/auth_product_contract/cleanup_contract.rscrates/ironclaw_auth/tests/auth_product_contract/oauth_flow_contract.rscrates/ironclaw_auth/tests/auth_product_contract/serde_redaction_contract.rscrates/ironclaw_first_party_extensions/assets/slack_user/manifest.tomlcrates/ironclaw_first_party_extensions/assets/slack_user/prompts/slack_user/get_conversation_history.mdcrates/ironclaw_first_party_extensions/assets/slack_user/prompts/slack_user/get_user_info.mdcrates/ironclaw_first_party_extensions/assets/slack_user/prompts/slack_user/list_conversations.mdcrates/ironclaw_first_party_extensions/assets/slack_user/prompts/slack_user/search_messages.mdcrates/ironclaw_first_party_extensions/assets/slack_user/prompts/slack_user/send_message.mdcrates/ironclaw_first_party_extensions/assets/slack_user/schemas/slack_user/get_conversation_history.input.v1.jsoncrates/ironclaw_first_party_extensions/assets/slack_user/schemas/slack_user/get_user_info.input.v1.jsoncrates/ironclaw_first_party_extensions/assets/slack_user/schemas/slack_user/list_conversations.input.v1.jsoncrates/ironclaw_first_party_extensions/assets/slack_user/schemas/slack_user/raw_output.v1.jsoncrates/ironclaw_first_party_extensions/assets/slack_user/schemas/slack_user/search_messages.input.v1.jsoncrates/ironclaw_first_party_extensions/assets/slack_user/schemas/slack_user/send_message.input.v1.jsoncrates/ironclaw_first_party_extensions/assets/slack_user/wasm-src/Cargo.tomlcrates/ironclaw_first_party_extensions/assets/slack_user/wasm-src/src/api.rscrates/ironclaw_first_party_extensions/assets/slack_user/wasm-src/src/lib.rscrates/ironclaw_first_party_extensions/assets/slack_user/wasm-src/src/types.rscrates/ironclaw_first_party_extensions/src/gsuite/account_policy.rscrates/ironclaw_host_api/src/http.rscrates/ironclaw_host_runtime/src/egress/mod.rscrates/ironclaw_host_runtime/src/egress/pipeline.rscrates/ironclaw_host_runtime/tests/github_wasm_runtime_contract.rscrates/ironclaw_host_runtime/tests/runtime_http_egress_contract.rscrates/ironclaw_reborn_composition/src/auth.rscrates/ironclaw_reborn_composition/src/available_extensions.rscrates/ironclaw_reborn_composition/src/credential_refresh_worker.rscrates/ironclaw_reborn_composition/src/extension_lifecycle.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/google_oauth/mod.rscrates/ironclaw_reborn_composition/src/input.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/nearai_mcp.rscrates/ironclaw_reborn_composition/src/notion_oauth.rscrates/ironclaw_reborn_composition/src/oauth_dcr.rscrates/ironclaw_reborn_composition/src/oauth_gate.rscrates/ironclaw_reborn_composition/src/oauth_provider_client.rscrates/ironclaw_reborn_composition/src/oauth_provider_client/tests.rscrates/ironclaw_reborn_composition/src/product_auth_durable.rscrates/ironclaw_reborn_composition/src/product_auth_durable/cleanup.rscrates/ironclaw_reborn_composition/src/product_auth_durable/flows.rscrates/ironclaw_reborn_composition/src/product_auth_durable/tests.rscrates/ironclaw_reborn_composition/src/product_auth_providers.rscrates/ironclaw_reborn_composition/src/product_auth_serve/lifecycle.rscrates/ironclaw_reborn_composition/src/product_auth_serve/mod.rscrates/ironclaw_reborn_composition/src/product_auth_serve/oauth.rscrates/ironclaw_reborn_composition/src/projection/display_preview.rscrates/ironclaw_reborn_composition/src/projection/tests/turn_stream_auth.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/src/slack_actor_identity.rscrates/ironclaw_reborn_composition/src/slack_channel_connection.rscrates/ironclaw_reborn_composition/src/slack_channel_routes.rscrates/ironclaw_reborn_composition/src/slack_channel_routes/allowed/tests.rscrates/ironclaw_reborn_composition/src/slack_channel_routes/setup.rscrates/ironclaw_reborn_composition/src/slack_connectable_channel.rscrates/ironclaw_reborn_composition/src/slack_host_beta/runtime_setup.rscrates/ironclaw_reborn_composition/src/slack_personal_binding.rscrates/ironclaw_reborn_composition/src/slack_personal_oauth.rscrates/ironclaw_reborn_composition/src/slack_setup.rscrates/ironclaw_reborn_composition/src/test_support/oauth_product_auth.rscrates/ironclaw_reborn_composition/tests/auth_callbacks.rscrates/ironclaw_reborn_composition/tests/auth_lifecycle.rscrates/ironclaw_reborn_composition/tests/webui_v2_product_auth.rstests/support/reborn_parity_qa/qa_trace.rstools-src/slack_user/Cargo.tomltools-src/slack_user/README.mdtools-src/slack_user/slack_user-tool.capabilities.jsontools-src/slack_user/src/api.rstools-src/slack_user/src/lib.rstools-src/slack_user/src/types.rs
|
/canary |
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Verdict: ❌ Changes requested
Findings: 1 blocking / 0 notes
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Head: 384ded211204b99a77cba18264a4b02e95b57d76
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found a blocking integration mismatch that prevents the new slack_user extension from starting its own Slack personal OAuth setup flow.
Findings
1. ❌ [MEDIUM] Slack personal OAuth rejects the new slack_user extension
Location: crates/ironclaw_reborn_composition/src/product_auth_serve/oauth.rs:208
The Slack personal OAuth start path only accepts requester_extension == "slack", but the new first-party personal Slack package added by this PR is id "slack_user" and its runtime credential requirements use requester_extension "slack_user". The WebUI extension setup route is keyed by package id, so starting OAuth for the new slack_user extension will hit this branch and return invalid_request before creating the flow. The tests exercise Path("slack"), which misses the actual new extension id. Allow slack_user here or route/setup the manifest through the accepted id, and add a caller-level test for /api/webchat/v2/extensions/slack_user/setup/oauth/start.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
|
Started Reborn WebUI v2 live canary for |
discard_oauth_egress_policy's doc says a discard failure "is logged", but the oauth_execution_context arm did `Err(_) => return` silently — inconsistent with the sibling handler.abort branch that warns. Add the warn! so the best-effort cleanup failure is diagnosable (error-handling.md: don't drop the cause). [skip-regression-check] log-only change; no testable behavior change (best-effort cleanup path, outcome unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Quick-scan pass (CLAUDE.md Testing Discipline rule 3 / |
# Conflicts: # crates/ironclaw_reborn_composition/src/input.rs # crates/ironclaw_reborn_composition/src/lib.rs # crates/ironclaw_reborn_composition/src/product_auth/api/auth.rs # crates/ironclaw_reborn_composition/src/product_auth/credentials/product_auth_providers.rs # crates/ironclaw_reborn_composition/src/product_auth/oauth/google_oauth/mod.rs # crates/ironclaw_reborn_composition/src/product_auth/oauth/notion_oauth.rs # crates/ironclaw_reborn_composition/src/product_auth/oauth/oauth_dcr.rs # crates/ironclaw_reborn_composition/src/product_auth/serve/mod.rs # crates/ironclaw_reborn_composition/src/product_auth/serve/oauth.rs # crates/ironclaw_reborn_composition/src/projection/tests/turn_stream_auth.rs # crates/ironclaw_reborn_composition/src/test_support/oauth_product_auth.rs
…5643) * ci(reborn): run all webui_v2 JS tests in CI; drop deleted e2e ref - Broaden the reborn-tests.yml JS-test glob from static/js/pages/settings to all of crates/ironclaw_webui_v2/static/js (minus node_modules/dist), so the chat/extensions/lib suites run in CI. - Stub DOMPurify.addHook in markdown.test.mjs: renderMarkdown registers a one-time afterSanitizeAttributes hook before sanitizing, so the mock must accept the registration for the sanitize assertion to be exercised. - Drop tests/e2e/scenarios/test_reborn_webui_v2_legacy_channel_connect.py from the reborn-playwright legacy-auth-inputs group (the scenario is removed later in this stack). Mechanical re-slice (1/4) of the fully-reviewed #5604 head 178829a. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(reborn): Slack personal OAuth foundations (dormant, additive) Additive layer for Slack personal OAuth; no user-visible change. The catalog still offers pairing, all pairing endpoints remain intact, and the new OAuth surface stays dormant until the serve/webui wiring lands in the next PR of this stack. - ironclaw_auth: OAuth primitives generalized beyond Google (OAuthCallbackState newtype, OAuthRedirectUri), provider_identity on credential accounts, SLACK_PERSONAL constants, owner-granularity cleanup + provider selector, contract tests. - oauth_provider_client: SlackAuthedUser parsing, expires_in=0 (non-expiring token) fix, exchange logging. - host_api http + host_runtime egress: execute_credential_exchange with fail-closed enforcement + runtime egress contract tests. - oauth_gate: unified OAuth gate driver (Google refactored onto it, OAuthGateProviderRegistry) incl. fallthrough fix; ripple into google_oauth/notion_oauth/oauth_dcr/nearai_mcp. - slack_personal_oauth (new) + product_auth_serve: Slack OAuth start/callback routes, identity hook, failure-HTML signal. - slack_setup: oauth client-id/secret slot + personal_oauth_ready; slack_channel_connection; SlackPersonalUserBinder trait; product_auth_durable cleanup/flows. - slack_user tool: full WASM user-token tool (tools-src + first_party_extensions assets incl. committed wasm). - gsuite account_policy ctor ripple; provider_identity field ripple across struct literals. Transitional #[allow(dead_code)] markers sit on four pub(crate) hooks that the serve/slack_host_beta wiring consumes in the next stack PR; they disappear when those files reach their final reviewed state. Mechanical re-slice (2/4) of the fully-reviewed #5604 head 178829a. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(reborn): swap Slack pairing codes for personal OAuth The accepted swap: Slack relay pairing is retired; users connect via the personal OAuth flow added by the previous PR of this stack. - Remove slack_personal_binding_pairing{,_serve}, slack_pairing_notifier, channel_connection_resume (composition + product_workflow), the /pair slash-command endpoint, and the v1 pairing_approve builtin tool + registry wiring. - host_api: remove the ChannelPairing runtime credential setup variant, adding a tolerant #[serde(other)] Retired variant + wire test so persisted legacy rows still decode. - reborn_cli serve wiring: fill the Slack personal OAuth slot; drop the commands mount and pairing route config. serve_slack.rs loses the legacy env-based setup import path; legacy [slack] fields are ignored here, and the explicit startup rejection lands in the final stack PR. - WebUI v2: OAuth Configure flow, in-chat OAuth card, watchers, shared product-auth-oauth-events lib + tests; pairing JS deleted; i18n x11 locales; asset manifest updated. - e2e scenarios updated; legacy channel-connect scenario removed. - CHANGELOG: dm_policy default pairing->allowlist + Removed entries; registry/channels/slack.json bump; docs/plans updates. Behavior (owner-accepted, CHANGELOG'd): previously-paired v1 Slack users are force-unpaired and reconnect via OAuth; Telegram/WASM self-service pairing via the pairing endpoints is unaffected. Mechanical re-slice (3/4) of the fully-reviewed #5604 head 178829a. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(reborn-cli)!: reject legacy [slack] config fields at serve startup BREAKING: ironclaw-reborn serve now rejects the legacy [slack] config fields (installation_id, team_id, api_app_id, slack_user_id, user_id, shared_subject_user_id, signing_secret_env, bot_token_env, channel_routes) with an actionable error instead of silently ignoring them. Slack bot credentials and routing are configured from the WebUI channel setup page; per-user identity comes only from Slack OAuth. [slack].enabled / IRONCLAW_REBORN_SLACK_ENABLED still gate whether the channel mounts. Shipped last in the stack and independently revertable: reverting this commit alone restores the silent-ignore behavior without touching the OAuth swap. Includes the reject_legacy_slack_setup_fields regression test and the CHANGELOG Breaking entry. Mechanical re-slice (4/4) of the fully-reviewed #5604 head 178829a. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(reborn): rename JS-test step to match broadened webui scope Review follow-up on #5643 (CodeRabbit): the step name and empty-result message still said "settings" after the find glob broadened to all of crates/ironclaw_webui_v2/static/js. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(reborn): close review findings on Slack OAuth foundations (#5644) Review follow-ups from the multi-agent + Gemini review of the foundations slice: - slack_setup: return the client secret SecretString directly instead of expose->to_string->rewrap (SecretMaterial is a SecretString alias; drops a needless plaintext heap copy). - slack_setup: rollback_failed_activation_save now also deletes the failed save's fresh oauth_client_secret_handle (guarded by the same previous/protected-current reference check as bot/signing handles); extended the rollback test and added an inherited-handle regression. - ironclaw_auth: OAuthProviderIdentitySubject deserializes through validation (#[serde(try_from = "String")] + TryFrom delegating to new) per the types.md newtype rule; the type is new in this stack so no persisted rows predate the tightening. - oauth_provider_client + oauth_dcr: consume the staged network policy after every credential exchange (success or failure) via the obligation handler's abort seam; the egress pipeline only discards on pre-transport errors, so successful exchanges leaked one unbounded policy-store entry per flow. Regression asserts the staged policy is discarded after a successful exchange. - product_auth_serve: the Slack callback identity hook now returns a compensating rollback; if complete_oauth_callback fails after the hook durably bound the Slack identity, the completion-failure arm deletes exactly that binding (scoped by full provider_user_id) so a failed completion cannot leave Slack "connected" with no usable credential. Threaded a RebornUserIdentityBindingDeleteStore through SlackPersonalOAuthBindingConfig; regression drives the route-level harness with a failing completion and asserts bind-then-rollback. - slack_user tool: log only the static Slack API resource name (strip the query string carrying search terms / channel ids / timestamps) in both source copies; committed wasm rebuilt with cargo-component (wasm32-wasip2). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(reborn): close review findings on the Slack OAuth swap (#5645) Review follow-ups from the multi-agent + Gemini review of the swap slice: - serve_slack: restore the fail-loud guard for featureless builds. The swap had left the non-slack-v2-host-beta branch returning Ok(None) unconditionally, silently starting without Slack when [slack].enabled=true / IRONCLAW_REBORN_SLACK_ENABLED=true. The enablement helpers are no longer feature-gated and the cfg(not) variant bails with the build-feature message; cfg(not) regression tests reinstated plus a unit test for the shared bool parser. - configure-modal: a blocked about:blank pre-open now surfaces "Authorization popup was blocked." and skips the OAuth mutation instead of burning the server-side flow with no completion watcher (mirrors the in-chat startOnboardingOAuth guard); tests drive the captured authorize handler for both the blocked and unblocked paths. - webui_v2_product_auth: caller-level serve tests for the Slack personal OAuth wiring — bearer-required 401, start through the composed router (asserting the Slack authorize URL + server-side user_scope), fail-closed 503 backend_unavailable when product auth is mounted without the slot (the exact state a dropped webui_serve wiring block would produce), and callback mounted + fail-closed sanitized. Backed by a tests-only filled-slot seam on SlackPersonalSetupServiceSlot mirroring the production fill path. - slack_host_beta: thread the mounts' user_identity_delete_store into SlackPersonalOAuthBindingConfig (completes the callback binding rollback landed in the foundations slice). - runtime_setup: document that the setup-save hook deliberately activates the channel only — the slack_user companion requires a caller-scoped slack_personal account and is owned by the post-OAuth activation path. - .env.example: document IRONCLAW_REBORN_SLACK_PERSONAL_OAUTH_REDIRECT_URI alongside the Reborn Slack serve settings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(reborn): pin slack token-parse failures to TokenExchangeFailed Review follow-up from #5604 (Gemini): the two slack parse-failure tests asserted only is_err(), so an unrelated parse failure could keep them green. Both now assert the specific AuthErrorCode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(reborn): close manual-E2E findings on the Slack OAuth flows Four findings from live end-to-end testing of the stack: - Restore the pair-once-resume-all behavior OAuth lost in the swap: a completed auth flow's continuation references at most one run (TurnGateResume) or none (SetupOnly from the Settings surface), so authorizing Slack in one chat left the caller's other chats parked on BlockedAuth — the deleted channel_connection_resume machinery was the only cross-thread fan-out and nothing replaced it. A new BlockedAuthResumeFanout decorator wraps the continuation dispatcher: after the primary dispatch it scans the durable turn-state snapshot for the caller's other BlockedAuth runs whose credential requirements name the completed flow's provider and resumes each (best-effort, idempotent per flow+run, strict tenant+owner scoping, primary run skipped). Provider-keyed, so multiple chats blocked on Google resume together too — pairing-era parity, generalized. AuthContinuationEvent now carries the completed flow's provider so dispatchers can fan out without re-reading the flow record. Production-shaped builders keep the single-run dispatcher (explicit None) until their snapshot source is wired. - auth-oauth-card: open authorization in a sized popup instead of a new tab — pre-open about:blank with window features, sever the opener, navigate via the shared openAuthPopup reuse path (gate completion travels over localStorage/BroadcastChannel, never window.opener), and surface a blocked popup instead of silently doing nothing. Matches the onboarding and configure flows. - auth-oauth-card: render provider display names ("Slack") instead of naively capitalized raw ids ("Slack_personal") in the authorize CTA and gate shell, with an underscores-to-title-case fallback. - Spinners: the OAuth surfaces used Tailwind's animate-spin, which app.css never defines and whose static-motion policy would suppress anyway; switch to the sheet's sanctioned v2-spin class (the same one the automations refresh spinner uses). Tests: three fan-out regressions (turn-gate completion resumes only the caller's other provider-blocked runs; SetupOnly completion resumes all; resume failures stay best-effort), five auth-oauth-card component tests (display name, prettified unknown ids, sized popup + severed opener + in-place navigation, blocked-popup surface, non-HTTPS refusal), spinner class pins updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(reborn): let Slack disconnect succeed when no workspace setup exists Live-testing follow-up: POST /extensions/slack/remove returned 500 on a fresh instance because disconnect_channel_for_caller fails closed when the personal connection scope is unresolvable — but a never-configured (or setup-deleted) instance has no installation scope at all, so extension uninstall was impossible before Slack was ever set up. The no-scope arm now still revokes the caller's provider-scoped slack_personal credentials, deletes the caller's own Slack identity bindings without an installation prefix (the delete stays tenant + caller-user bound), skips DM targets (installation-keyed and unreachable without a setup), and succeeds. The scoped path is unchanged; the shared cleanup request moved into a helper so the two arms cannot drift. Updated the pinned test to the new contract: disconnect-without-scope succeeds and cleans the caller's bindings unscoped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(reborn): design spec for Slack bot/tools remodel Approved design-of-record for the model-B remodel (bot = operator entrypoint, tools = user-installable extension) and the three stacked follow-up PRs: remodel / least-privilege scopes / OAuth durability. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): rename Slack extensions (bot slack->slack_bot, tools slack_user->slack) Mechanical rename only, no behavior change - commit 1 of the model-B remodel. Bot channel extension id -> slack_bot; user-tools extension id -> slack (the visible Slack extension). Renames id constants, manifest ids, asset dirs, include paths, and capability ids (slack_user.*->slack.*); cross-crate test refs updated. Unchanged: slack_user_token/slack_bot_token handles, slack_personal provider, Slack adapter/channel and actor kinds, ironclaw_slack_v2_adapter crate, slack-v2-host-beta feature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(reborn): model-B Slack backend — bot is hidden channel, tools are the installable extension Commit 2 of the model-B remodel (backend behavior). The Slack tools extension (slack) becomes the visible, user-installable extension; the bot channel (slack_bot) becomes hidden operator infrastructure. - Visibility flip: is_internal_extension_package_ref now hides the bot (slack_bot), not the tools; the catalog/list surface the tools (slack). - Delete the ~150-line companion coupling (activate_slack_with_companion + 5 helpers + call-site branches). The tools extension is a normal visible extension activated directly via the standard lifecycle, so the connect-but-inactive activation-gap bug dissolves — there is no hidden companion that can silently fail to activate. - The tools extension owns the slack_personal OAuth: the credential requirement and the OAuth-start requester check target slack, not the bot. - Unbound-user greeting: a first-contact DM from a Slack user with no identity binding is greeted with a connect nudge (previously silently dropped) — no binding lookup, no agent turn, canned text only. - Operator flow: the bot is fully hidden from the extension catalog/list; operators configure it via the Slack setup panel. Test-first: rewrote the companion/connection-state/search tests for model B and flipped the unbound-user silence test to assert the nudge. All Slack behavior tests pass; the remaining composition failures are pre-existing sandbox scheduler/model-call timeouts plus one flaky trigger test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn-webui): Slack extension is the tools package, not a channel Commit 3 of the model-B remodel (frontend). No behavioral change — the frontend is already model-B-compatible (verified: extensions + chat JS suites pass, 324 tests). The Slack tools extension connects via the standard credential / auth-OAuth-card path (like Gmail); the channel-connection path was the bot's and is superseded by the commit-2 unbound-user greeting. Renames the now-misleading isSlackChannel flag in configure-modal.js to isSlackToolsExtension: under model B the visible slack extension is the user-tools package, not the bot channel. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): rebuild slack tools WASM component after capability rename The slack->slack_bot / slack_user->slack rename (e477699) updated the wasm-src dispatch keys (slack.search_messages, slack.send_message, ...) and the host-runtime contract test's capability id, but the compiled slack_user_tool.wasm was git-renamed at 100% similarity — never recompiled. The shipped binary still dispatched on the old slack_user.* ids, so every Slack tool call failed at runtime (OperationFailed) and host_runtime_services_injects_personal_xoxp_token_for_slack_user_search_capability failed at CI. Recompiled the component from the renamed source (cargo component build --release --target wasm32-wasip2) and redeployed the artifact. Both github_wasm_runtime_contract slack tests now pass. No source change accompanies this commit — the regression test already lives in github_wasm_runtime_contract.rs and this binary rebuild is precisely what makes it green, so [skip-regression-check]. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(reborn): least-privilege per-capability Slack scopes The Slack tools manifest previously declared the full personal-OAuth scope union (including chat:write) on every capability, so a read-only tool like search_messages advertised write access it never uses. Declare each capability's real scopes instead: - The four read tools (search_messages, list_conversations, get_conversation_history, get_user_info) request only the ten read scopes. - Only send_message keeps chat:write (eleven-scope union). The OAuth *setup* request (SLACK_PERSONAL_OAUTH_SETUP_SCOPES) still asks for the union up front — a single consent that covers every tool the user may call — because per-scope, opt-in-on-first-write consent is a larger UX effort tracked in #5669. This change makes the per-capability manifest truthful now and documents the follow-up at the constant. Tests: - available_extensions: slack_read_only_tools_do_not_request_chat_write asserts only send_message carries chat:write in the manifest. - github_wasm_runtime_contract: slack_user_scopes() drops chat:write so the read-capability injection fixtures match the read-only manifest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): compile composition tests without slack-v2-host-beta compose_provider_client_with_runtime takes a `#[cfg(feature = "slack-v2-host-beta")]`-gated 5th parameter (slack_personal_oauth_slot). The production wrapper gates the argument it forwards with the same cfg, but the four `#[tokio::test]` call sites in product_auth_providers.rs passed `None` unconditionally. Without the feature the function takes 4 args, so the tests supply one too many and fail to compile (E0061) — breaking `cargo test` and `cargo clippy --tests` in any default-feature build of this crate. Gate each test call site's 5th argument exactly like the production wrapper. The composition lib-tests now compile under both the default and the slack-v2-host-beta feature sets. Why CI stayed green until now: code_style.yml runs the SLIM clippy matrix (--all-features only) for pull_request and merge_group, where the feature is on and the tests compile. Only the FULL matrix on push-to-main adds the no-features `default` leg that trips E0061 — so this latent break would have turned main red after the Slack stack merged. That default leg is the regression guard for this fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(reborn): drop stray blank line left by /pair command removal Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): drop dead slack_manifest_toml() after model-B ingress repoint The Slack events host-ingress route now projects from the bot manifest (slack_bot_manifest_toml), so slack_manifest_toml() has no callers and trips clippy's dead_code lint under -D warnings. The SLACK_MANIFEST const it wrapped is still used directly for the tools package. Removed the dead wrapper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(reborn): durable Slack conversation binding (survives process restart) The Slack host-beta record builder constructed an in-memory `InMemoryConversationServices`, so conversation->thread bindings were lost on process restart (the removed `tracing::warn!` admitted exactly that). Thread the conversation services in as a parameter instead: the async production path (`runtime_setup::build_resolver`) now constructs a durable, filesystem-backed `RebornFilesystemConversationServices` over the shared host-state filesystem (backend = libSQL / Postgres / local disk, a property of the root filesystem, shared with the idempotency ledger), while the sync/test entrypoint keeps in-memory (no async context there to rehydrate). A `SlackConversationServices` bundle enforces that the `ConversationBindingService` and `ConversationActorPairingService` handles share one backing store. Coverage: durability is contract-tested by `ironclaw_conversations::filesystem_conversation_services_round_trip_persisted_state_on_reopen` (write a binding, reopen a fresh store over the same filesystem, read it back — the filesystem equivalent of the libSQL/Postgres restart-replay tests); the async Slack path building and routing DMs with the durable store in place is covered by the `slack_host_beta` runtime-mount regression tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): display the Slack tools extension as "Slack" Drop the stale "(personal)" qualifier from the tools package display name. It was inherited from the pre-model-B "slack_user" package, when a visible bot "Slack" and visible tools needed to be told apart. Model-B hides the bot, so the qualifier now contrasts with something the user cannot see. Rename the display name (manifest + the mirrored Rust literal), the 5 schema titles, and trim the description reference to the hidden bot channel. Identity is unchanged (id stays "slack"); this is display-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): match Slack tool asset dirs to the extension id Rename the tools schema/prompt asset directories from the legacy slack_user/ (the pre-model-B id) to slack/, matching the extension id and the house convention (github uses schemas/github/). Updates the git paths, the manifest input/output/prompt refs, and the include_bytes! mounts in slack_assets(). The WASM binary filename (slack_user_tool.wasm) and the slack_user_token credential handle keep their legacy names (identity/build artifacts, out of scope). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(reborn): rustfmt-collapse slack_package call after shortening the label Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): report all rejected legacy Slack fields at once Address CodeRabbit review on #5646: the legacy-config rejection in resolve_slack_config_for_serve short-circuited on the first deprecated [slack] field, so an operator fixing one field would rediscover the next only on the next boot. Collect every violated field (including channel_routes) into one bail message, and add a regression test that drives the channel_routes rejection branch through resolve_slack_config_for_serve. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): tidy Slack tools ext per CodeRabbit review on #5668 Three review nits on the model-B Slack tools extension: - get_user_info claimed it returns the user's email, but the granted scope is `users:read` (not `users:read.email`), so email is never available. Drop "email" from the capability description, the model-facing prompt doc, and the params doc comment so the model does not promise a field it cannot fetch. - action_from_context dropped the serde parse error via `map_err(|_| ...)`. Carry the cause (`invalid_invocation_context: {e}`) so a malformed invocation context is diagnosable. Recompiled slack_user_tool.wasm (wasm32-wasip2) with the change. - Extract the bare `"slack"` magic string in configure-modal.js into a named `SLACK_TOOLS_EXTENSION_ID` constant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): classify Slack conversation-store outage as 503, not 401 Address CodeRabbit review on #5693: when the durable filesystem conversation-binding store fails to initialize, build_resolver mapped the error to SlackIngressError::InstallationNotFound, which ingress_error_response renders as 401 Unauthorized. A storage/infra outage is not an authentication failure — a 401 tells Slack the installation is unauthorized (misleading, and Slack will not meaningfully retry), and the underlying cause was dropped. Add a dedicated SlackIngressError::ConversationStoreUnavailable variant that carries the cause, map the resolver error to it, and render it as 503 Service Unavailable (TemporarilyUnavailable) so Slack retries delivery. Add a regression test pinning the 503 mapping. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): revoke exclusive extension credential on removal Regression from the Slack pairing->OAuth swap (2ce8807). Before it, Slack used pairing-based, extension-owned credentials that removal cleaned up automatically, so a removed extension could not be silently re-added. OAuth personal credentials are stored `UserReusable` and are preserved across extension removal by default (the crate guardrail: reusable credentials are untouched unless a provider-scoped cleanup opts in). Extension removal did no credential cleanup at all, so after removal the agent re-installed the bundled extension and re-activated on the surviving token — no OAuth re-consent, a security boundary violation. The revocation lives on the single convergence point both removal entrypoints already call — `RebornLocalExtensionManagementPort::remove` (now `remove(package_ref, scope)`) — so neither door can bypass it: the WebUI facade (`LifecycleProductAction::ExtensionRemove`, the door users actually use) and the `builtin.extension_remove` agent capability both route through it. On success `remove` revokes the removed extension's credential providers that are exclusive to it, via the sanctioned `RebornProductAuthServices::cleanup_credentials_for_lifecycle` path (provider-scoped `Uninstall`). Shared vendor credentials are preserved: removing `gmail` does not revoke the `google` token `google-calendar`/ `drive` still use, determined by re-checking every still-installed extension's declared providers. Cleanup is best-effort (never fails or rolls back the removal) and fails safe (revokes nothing) when it cannot prove a provider is unused, so a shared credential is never deleted out from under another extension. Regression tests (both doors converge on the port): - ui_facade_extension_remove_revokes_exclusive_credential_at_convergence_point: drives the WebUI facade `ExtensionRemove` and asserts the port issues exactly one provider-scoped cleanup for the exclusive github provider. - local_dev_extension_remove_revokes_exclusive_credential_so_reactivation_requires_auth: drives the `builtin.extension_remove` agent capability, then asserts re-activate returns auth_required (previously re-activated silently). - local_dev_extension_remove_preserves_shared_credential_used_by_another_extension: gmail+calendar share google; removing gmail keeps calendar activatable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): origin-independent OAuth flow-status poll for reconnect The extension-setup "reconnect" modal could hang forever after OAuth completed. On reconnect the frontend watcher has no configured-state poll fallback (the extension is already configured), so it depends solely on the callback page's same-origin localStorage/BroadcastChannel signal. When the callback runs on a different origin (local ngrok callback vs 127.0.0.1 opener, or split app/callback domains in prod), that signal never reaches the opener tab and the modal never closes. Add a read-only, authenticated, caller-scoped flow-status endpoint (GET /api/reborn/product-auth/oauth/flow/{flow_id}/status) returning the durable AuthFlowStatus by id, and wire the reconnect watcher to poll it as an origin-independent backstop (fire-and-forget with a pending guard; the same-origin browser signal stays the fast path). The response carries the status enum only -- never tokens, PKCE verifiers, authorization codes, or opaque state. Ownership is enforced by get_flow full-scope equality; a flow that is unknown OR owned by another scope both return 404 so the read cannot be a cross-user existence oracle. The browser echoes back the invocation_id the start response minted (callback_scope.invocation_id) so the caller-scoped handler re-derives the exact scope get_flow matched on, while the trusted tenant/user still come from the authenticated caller. Regression coverage: frontend reconnect-with-no-browser-signal completes via the poll (and a failed poll surfaces a retryable error); backend caller-level tests lock completed->"completed" without secrets, malformed id->400, unknown id->404, and cross-scope->404 (not 403). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(reborn): post the renamed 'slack' extension id in personal OAuth start tests The model-B remodel renamed the installable Slack tools extension slack_user -> slack (and the bot channel slack -> slack_bot), but slack_personal_oauth_start_serves_through_composed_router / _fails_closed_without_slot still posted "slack_bot" — the hidden bot channel, which is not OAuth-installable — so the handler correctly rejected it (400) and the composed-router test asserted 200 and failed. Post "slack" so the tests exercise the real installable extension. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): unify activation button loading into the shared Button Both the in-extension (configure-modal) and in-chat (onboarding-pairing-card) activation flows rendered their own copy-pasted `spinnerGlyph()` — a small, cramped, filled quarter-arc glyph placed ad-hoc before the label. Replace both with a single clean `loading` state on the design-system Button: - Button gains a `loading` prop: a stroke-based ring + rounded-cap arc spinner (v2-spin, reduced-motion-safe), auto-disables the button, sets aria-busy, and keeps the label so width doesn't jump. - configure-modal (OAuth connect, pairing connect, save) and onboarding-pairing-card (configure, pairing submit) buttons now pass `loading=...` instead of hand-rolling the glyph; both `spinnerGlyph()` copies are deleted. - Tests updated to assert the button's `loading` prop (the spinner now lives inside Button) instead of the old `disabled`/`v2-spin` mechanism. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(reborn): loading state + popup-close feedback for the in-chat OAuth gate The in-chat auth-gate OAuth card only rendered "Open/Re-open Slack authorization" — no loading state, and no feedback if the user closed the popup before finishing (the extension Configure path already put the spinner on its button). Bring it to parity: - While the authorization popup is open, the primary button itself shows the shared clean loading spinner (Button `loading`) with a "waiting to authorize" label — same button, same color, no separate status row. - Watch the popup for closing; after a short grace window (a successful callback closes the popup itself, and the gate then clears via the completion signal / resumed-run projection), surface a "closed before you finished — re-open to try again" notice. - Extract the spinner into design-system/spinner.js so Button's loading state (used here and by the extension flow) shares one clean animation. Completion detection is unchanged (signal + projection backstop); this is UI feedback only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): register the /conversations mount alias for the Slack durable conversation store The Slack host-state ScopedFilesystem (built via slack_host_state_mount_view) registered aliases for slack-personal-binding, slack-channel-routes, slack-setup, and product_workflow/idempotency — but not /conversations. The durable conversation-binding store (RebornFilesystemConversationServices) persists /conversations/state.json, so every inbound Slack event — including a DM to the bot — failed to open the store and was dropped with a 503 ("no mount alias matches scoped path"). The split/7 durability wiring surfaced the failure correctly, but the store itself could never initialize. Add the /conversations alias (mapped to /tenants/{tenant}/shared/slack-conversations) and lock it with a regression case in the mount-view test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(reborn): coverage for OAuth disconnect/reconnect/durable-store + Button loading Lands the test-integration agent's coverage on split/7: - Rust (composition, through-the-caller): - ui_facade_extension_remove_preserves_credential_still_shared_with_another_extension (extension_lifecycle.rs) — the providers_still_in_use fail-safe: removing gmail must NOT revoke the google credential still used by google-calendar. - slack_durable_conversation_store_initializes_through_composed_host_state_mount (factory.rs) — drives local_dev_slack_host_state_filesystem -> RebornFilesystemConversationServices::new, asserting /conversations/state.json opens (caller-level guard for the mount-alias fix). - flow_status_route_descriptor_locks_read_only_bearer_policy (product_auth_serve/mod.rs) — locks the reconnect flow-status route as GET / NoBody / bearer-required / AuthenticatedCaller / per-caller / SameOriginOnly. - JS unit: - button.test.mjs — the shared Button `loading` prop (spinner + disabled + aria-busy; idle has none; both variant paths; disabled alone). - auth-oauth-card.test.mjs — button loading + "authorizing" label while the popup is open; closed-before-finish notice after the popup closes. - Fmt-only: webui_v2_product_auth.rs line unwrapped after the slack_bot->slack rename. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci(reborn): run reborn-tests on stacked PRs + add webui-v2 no-undef lint Two CI-scope gaps that let bugs through on the stacked Slack PRs: 1. reborn-tests.yml had `pull_request: branches: [main]`, so the entire Reborn test workflow (composition crate-tests, group-tests, webui-v2 JS tests, integration tier) skipped every PR whose base is a feature branch rather than main. A rename in one slice broke a composition test in another and no per-PR run caught it. Drop the branches filter (matching code_style.yml); the changes/classify job still scopes suites by path. merge_group/push stay main-only. 2. Add a `webui-v2-js-lint` (eslint no-undef) gate over static/js. The node --test component suites stub every collaborator through a vm context, so a module referencing an un-imported symbol still passes its unit test and only crashes at runtime. This gate is the regression guard for exactly that class; it immediately caught a pre-existing latent bug — useChatEvents.js calling isTerminalToolStatus without importing it (ReferenceError in the tool-activity upsert path, on main since 47772e4) — fixed here by adding the import. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(reborn): post the renamed 'slack' extension id in personal OAuth start tests Cascades fee5549 down to split/5 (the branch that owns the rename), so #5668's own CI goes green rather than the fix being stranded on split/7. The model-B remodel renamed the installable Slack tools extension slack_user -> slack (and the bot channel slack -> slack_bot), but slack_personal_oauth_start_serves_through_composed_router / _fails_closed_without_slot still posted "slack_bot" — the hidden bot channel, which is not OAuth-installable — so the handler correctly rejected it (400) and the composed-router test asserted 200 and failed. Post "slack" so the tests exercise the real installable extension. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): map extension-lifecycle call failures to OperationFailed, not InputEncode Asked "is my slack connected?" after removing Slack, the agent escalated extension_search -> extension_activate("slack"). Activating a not-installed extension returns ProductWorkflowError::InvalidBindingRequest ("available extension was not found"), which lifecycle_error() mapped to RuntimeDispatchErrorKind::InputEncode -> the model saw the nonsensical "the tool input could not be encoded" / invalid_input, with a "requires_changed_input" retry hint, and relayed it to the user. A failure from a lifecycle CALL (activation requirements, activate/install/ remove) is an operation-level failure, not a tool-input encoding problem. Map InvalidBindingRequest / UnsupportedActionKind (and keep Transient/other) to OperationFailed ("the tool operation failed"). The old code also dropped the reason via `.map_err(|_| ...)`; log it at debug! (server-side) since the fixed model-visible summary cannot carry the raw reason (safe-summary invariant), so the failure stays diagnosable instead of vanishing. Input-parsing failures (parse_input, extension_package_ref) keep InputEncode — those are genuinely malformed tool input. Regression: lifecycle_error_maps_call_failures_to_operation_failed_not_input_encode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): gate the Slack connect-nudge to 1:1 DMs, never shared channels post_connect_nudge_if_unbound_user_message fired for any UserMessage that rejected with BindingRequired — the doc comment said "first-contact DM" but the code never verified it. An unbound user's app-mention in a SHARED channel also rejects with BindingRequired, so the host connect-nudge ("connect your Slack account…") got posted INTO the shared channel, dropping a message addressed to one user where the whole channel sees it. Gate the nudge on the conversation being a 1:1 DM: Slack DM (im) channel ids start with 'D'; shared channels ('C') and multi-person/group DMs ('G') are excluded, and a missing/blank conversation ref fails closed. Matches the existing slack_reply_target_is_personal_dm 'D'-prefix convention. Regression: rejected_unbound_user_message_in_shared_channel_posts_no_connect_nudge (shared channel -> zero posts); the existing DM test still posts exactly one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Revert "fix(reborn): map extension-lifecycle call failures to OperationFailed, not InputEncode" This reverts commit 61c35a6. * fix(reborn): add authGate.authorizing to all locales for i18n key parity The in-chat OAuth card added `authGate.authorizing` ("Waiting for {provider}…") to en.js only. The i18n_consistency test (`all_locales_share_the_en_key_set`) requires every locale to share en's key set, so it drifted — surfaced now that the CI-scope fix runs the webui-v2 crate bucket on stacked PRs. Add a translation to all 10 non-en locales (ar/de/es/fr/hi/ja/ko/pt-BR/uk/zh-CN). Regression guard: crates/ironclaw_webui_v2/tests/i18n_consistency.rs all_locales_share_the_en_key_set (already present; now green). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(reborn): rustfmt webui_v2_product_auth after slack_bot->slack shortening The #22 fix shortened `post_extension_oauth_start(&app, "slack_bot", ...)` to `"slack"`, which makes the call fit on one line; the two-line wrap it left behind fails `cargo fmt --check`. Normalize it (split/7 already had the one-line form via the cascade conflict resolution, which is why only #5668/ #5670 were red on Formatting). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci(reborn): require webui-v2-js-lint in the code-style rollup The webui-v2-js-lint (no-undef) job was added but never wired into the `code-style` aggregate's `needs:` + must-succeed result loop, so a JS no-undef regression would run but not block merge. Add it to both so the gate this PR introduced actually gates. It sits in the has_code-guarded must-succeed loop (the job runs whenever has_code is true, the same guard the rollup already short-circuits on). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): drop slack_actor from triggered_delivery_outcome after OAuth-only merge main's triggered_delivery_outcome.rs set SlackHostBetaConfig.slack_actor: None, but this stack removed the slack_actor field (Option<ExternalActorRef>, a preselected Slack user) as part of the pairing->OAuth swap — OAuth-only uses durable personal bindings, no preselected user. The main-merge brought the test but kept the stack's fieldless struct, so it failed E0560 (breaking Clippy(all-features) + the integration-coverage job). Remove the stale field. Verified: cargo test --no-run --test reborn_integration_triggered_delivery_outcome --all-features now compiles. [skip-regression-check] merge-cleanup: removes a reference to a stack-removed field so a main-added test compiles; no behavior change to test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): log discarded OAuth egress-policy execution-context error discard_oauth_egress_policy's doc says a discard failure "is logged", but the oauth_execution_context arm did `Err(_) => return` silently — inconsistent with the sibling handler.abort branch that warns. Add the warn! so the best-effort cleanup failure is diagnosable (error-handling.md: don't drop the cause). [skip-regression-check] log-only change; no testable behavior change (best-effort cleanup path, outcome unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): log dropped serde errors in extension lifecycle capability output Two silent-failure sites (error-handling.md): the dispatch output serialization mapped its serde error to OutputDecode via map_err(|_| ...) (cause dropped), and channel_connection_display_preview did serde_json::to_string(requirement).ok()? which silently returns None on failure — meaning the in-chat OAuth connection panel would silently never open. Add debug! logs before each so both are diagnosable; behavior is unchanged (still OutputDecode / still skips the preview, just no longer silently). [skip-regression-check] log-only diagnostics; no testable behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): post Slack connect nudge on the workflow-error path An unbound user's first-contact DM resolves as a `BindingRequired` workflow error (ScopeNotFound -> status 404), which the runner routes to `observe_workflow_error`, NOT `observe_workflow_ack`. The connect nudge was wired only into `observe_workflow_ack`, so an unbound 1:1 DM got total silence instead of the "connect your Slack account" prompt -- the nudge had never actually fired in production. Wire `post_connect_nudge_if_unbound_user_message` into `observe_workflow_error` too, mirroring the ack-path ordering: authorized rejection hint first, then the connect nudge for unbound 1:1 DMs. Regression test `unbound_user_message_via_workflow_error_posts_connect_nudge` drives the real error observer path -- the coverage that was missing. The existing test only called `observe_workflow_ack` directly with a synthetic `Rejected` ack, masking the gap (the repo's "test through the caller, not just the helper" rule). Proven red -> green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Fix Slack OAuth live canary setup * Tighten Slack OAuth live canary checks * Seed Slack OAuth redirect for live QA * Redact Slack setup API failure bodies * Require real Slack personal auth in live QA * Accept Slack admin setup surface in live QA * Harden Slack live QA credential guards * Address Slack OAuth foundation review * Address Slack OAuth swap review * Cover legacy Slack config rejection in serve * Clean up retired Slack user install on restore * Use effect metadata for Slack write-scope test * Address Slack OAuth durability review * Implement AsRef for OAuth identity subject * Align channel install toast test * Restore pairing card i18n test harness * Address Slack live canary review feedback * fix(hooks): bound libsql predicate connections under write lock * Harden live canary Playwright install * test(reborn): align Playwright expectations with Slack setup remodel * test(reborn): update Slack canary setup label * Fix WebUI v2 lint workflow after pnpm migration * Enable pnpm before WebUI lint cache setup * Fix QA 7C canary sheet prompt --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: firat.sertgoz <firat.sertgoz@near.ai> Co-authored-by: serrrfirat <f@nuff.tech>
What
Additive foundations for Slack personal OAuth — dormant by design (77 files). Both flows coexist after this merge: the catalog still offers pairing, all pairing endpoints stay intact, and nothing user-visible changes until PR 3 flips the surface.
ironclaw_auth— OAuth primitives generalized beyond Google (OAuthCallbackStatenewtype refactor,OAuthRedirectUri),provider_identityon credential accounts,SLACK_PERSONALprovider constants, owner-granularity cleanup + provider selector incleanup.rs/fakes.rs, contract-test updates.oauth_provider_client.rs—SlackAuthedUserresponse parsing,expires_in: 0(non-expiring token) fix, exchange logging.host_api/http.rs+host_runtimeegress —execute_credential_exchangehost call with fail-closed enforcement, runtime HTTP egress contract tests.oauth_gate.rs— unified OAuth gate driver (Google refactored onto it;OAuthGateProviderRegistryreplaces the Google-specific registry) incl. the gate fallthrough fix; ripple intogoogle_oauth,notion_oauth,oauth_dcr,nearai_mcp.slack_personal_oauth.rs(new),product_auth_serve— Slack OAuth start/callback routes, identity hook, failure-HTML signal. Routes are compiled in but not yet wired into the serve config (that wiring lands in PR 3).slack_setup.rs— OAuth client-id/secret slot on the installation setup +personal_oauth_ready;slack_channel_connection.rs,SlackPersonalUserBindertrait,product_auth_durablecleanup/flows.slack_usertool — the whole WASM user-token tool:tools-src/slack_user+crates/ironclaw_first_party_extensions/assets/slack_user(manifest, prompts, schemas, wasm-src, committed.wasm).account_policy.rs— constructor ripple from the new credential field.composition/lib.rs,extension_lifecycle.rs,slack_personal_binding.rs,available_extensions.rs,slack_host_beta/runtime_setup.rs,slack_connectable_channel.rs): only the additive hunks land here; their final states (with pairing removals) land in PR 3.Invariant
No user-visible change. Pairing remains fully functional; the new OAuth surface is dormant until PR 3 wires and exposes it. Merge PR 3 within days to keep the coexistence window short.
Stack
split/1-ci-js-tests— CI/JS test coveragesplit/3-slack-pairing-to-oauth-swap— the pairing→OAuth swapsplit/4-slack-legacy-config-rejection— legacy[slack]config rejectionMechanical re-slice of the fully-reviewed #5604 head
178829a4c; the stack tip is byte-identical to that head (git diff 178829a4c <stack-tip>is empty). See #5604 for the review history.Verification (local, at this slice)
cargo test -p ironclaw_reborn_composition --features webui-v2-beta,slack-v2-host-beta,test-support,libsql --libcargo test -p ironclaw_auth;cargo test -p ironclaw_host_runtime --test runtime_http_egress_contract;cargo test -p ironclaw_webui_v2 --features webui-v2-beta;cargo test -p ironclaw_architecture--all-targets -- -D warnings; reborn_cli + product_workflow compile checksscripts/check_no_panics.py: OKAll pass. WASM artifact: the committed
slack_user_tool.wasmis the reviewed build from #5604; the "WASM WIT Compatibility" CI check validates it.🤖 Generated with Claude Code