Conversation
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
|
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:
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 |
github-actions
Bot
added
size: XL
![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 unifies the previously separate Slack bot channel (slack_bot) and Slack personal tools (slack_personal) into a single, unified slack extension. This includes updating manifests, tests, and configurations, as well as introducing one-time forward migrations for legacy installation states and credential accounts. The review feedback highlights opportunities to make the TOML parsing more robust, handle potential errors during installation migration to prevent silent state loss, and use debug! logging instead of warn! to avoid corrupting REPL/TUI interfaces.
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.
| state | ||
| .manifests | ||
| .retain(|record| !record.raw_toml.contains("\nid = \"slack_bot\"")); |
There was a problem hiding this comment.
String matching on TOML using contains("\\nid = \"slack_bot\"") is fragile and can easily fail if formatting changes (e.g., spaces around =, different quote characters, or carriage returns). It is safer and more robust to parse the TOML line-by-line and normalize the key-value pair. When canonicalizing, perform cheaper validation checks on a trimmed slice before performing more expensive operations like replace that allocate a new string.
state
.manifests
.retain(|record| {
!record.raw_toml.lines().any(|line| {
let trimmed = line.trim();
if trimmed.is_empty() || !trimmed.contains("id") {
return false;
}
let clean = trimmed.replace(" ", "");
clean == "id=\"slack_bot\"" || clean == "id='slack_bot'"
})
});References
- When canonicalizing a string, perform cheaper validation checks (like length, emptiness, and character set) on a trimmed slice before performing more expensive operations like
replacethat allocate a new string. This avoids unnecessary allocations for already-invalid inputs.
| let has_unified_record = state | ||
| .manifests | ||
| .iter() | ||
| .any(|record| record.raw_toml.contains("\nid = \"slack\"")); |
There was a problem hiding this comment.
Similarly, checking for the unified manifest using contains("\\nid = \"slack\"") is fragile. Normalize the line-by-line TOML matching here as well, performing cheaper validation checks on a trimmed slice before performing more expensive operations like replace that allocate a new string.
let has_unified_record = state
.manifests
.iter()
.any(|record| {
record.raw_toml.lines().any(|line| {
let trimmed = line.trim();
if trimmed.is_empty() || !trimmed.contains("id") {
return false;
}
let clean = trimmed.replace(" ", "");
clean == "id=\"slack\"" || clean == "id='slack'"
})
});References
- When canonicalizing a string, perform cheaper validation checks (like length, emptiness, and character set) on a trimmed slice before performing more expensive operations like
replacethat allocate a new string. This avoids unnecessary allocations for already-invalid inputs.
| if let Ok(merged) = ExtensionInstallation::new( | ||
| existing.installation_id().clone(), | ||
| unified_id.clone(), | ||
| activation, | ||
| ExtensionManifestRef::new(unified_id, None), | ||
| bindings, | ||
| chrono::Utc::now(), | ||
| ) { | ||
| *existing = merged; | ||
| } | ||
| } else if let Some(first) = retired.into_iter().next() | ||
| && let Ok(renamed) = ExtensionInstallation::new( | ||
| first.installation_id().clone(), | ||
| unified_id.clone(), | ||
| first.activation_state(), | ||
| ExtensionManifestRef::new(unified_id, None), | ||
| first.credential_bindings().to_vec(), | ||
| chrono::Utc::now(), | ||
| ) | ||
| { | ||
| state.installations.push(renamed); | ||
| } |
There was a problem hiding this comment.
If ExtensionInstallation::new fails, the error is silently ignored. Since the retired installation was already removed from state.installations, the user's Slack installation state will be silently lost. It is safer to log an error using tracing::debug! so that any migration failures are visible and traceable without corrupting the REPL/TUI.
match ExtensionInstallation::new(
existing.installation_id().clone(),
unified_id.clone(),
activation,
ExtensionManifestRef::new(unified_id, None),
bindings,
chrono::Utc::now(),
) {
Ok(merged) => *existing = merged,
Err(error) => {
tracing::debug!(%error, "failed to merge retired slack_bot installation into unified slack extension");
}
}
} else if let Some(first) = retired.into_iter().next() {
match ExtensionInstallation::new(
first.installation_id().clone(),
unified_id.clone(),
first.activation_state(),
ExtensionManifestRef::new(unified_id, None),
first.credential_bindings().to_vec(),
chrono::Utc::now(),
) {
Ok(renamed) => state.installations.push(renamed),
Err(error) => {
tracing::debug!(%error, "failed to rename retired slack_bot installation to unified slack extension");
}
}
}References
- Do not use
warn!orinfo!logging in REPL/TUI-reachable code as they can corrupt the REPL/TUI. Usedebug!logging instead, and ensure only sanitized identifiers are logged.
| pub(crate) async fn migrate_retired_slack_personal_provider(&self) -> usize { | ||
| let retired: Vec<CredentialAccount> = self | ||
| .sweep_all_accounts() | ||
| .await | ||
| .into_iter() | ||
| .filter(|account| account.provider.as_str() == "slack_personal") | ||
| .collect(); | ||
| let mut migrated = 0usize; | ||
| for mut account in retired { | ||
| match ironclaw_auth::AuthProviderId::new("slack") { | ||
| Ok(provider) => account.provider = provider, | ||
| Err(error) => { | ||
| tracing::warn!(%error, "slack provider migration: unified provider id invalid"); | ||
| return migrated; | ||
| } | ||
| } | ||
| match self.write_account(&account, CasExpectation::Any).await { | ||
| Ok(_) => migrated += 1, | ||
| Err(error) => { | ||
| tracing::warn!( | ||
| account_id = %account.id, | ||
| %error, | ||
| "slack provider migration: failed to rewrite account record" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| migrated | ||
| } |
There was a problem hiding this comment.
When implementing backward compatibility or migration logic, avoid hardcoding specific provider IDs like slack or slack_personal. Instead, prefer checking for a common property (such as a specific environment variable) that defines the group of providers needing the compatibility logic. Also, use debug! logging instead of warn! to avoid corrupting the REPL/TUI.
pub(crate) async fn migrate_retired_providers(&self) -> usize {
let Ok(target_provider_str) = std::env::var("UNIFIED_AUTH_PROVIDER") else {
return 0;
};
let Ok(provider) = ironclaw_auth::AuthProviderId::new(&target_provider_str) else {
return 0;
};
let retired_provider_str = std::env::var("RETIRED_AUTH_PROVIDER").unwrap_or_default();
let retired: Vec<CredentialAccount> = self
.sweep_all_accounts()
.await
.into_iter()
.filter(|account| account.provider.as_str() == retired_provider_str)
.collect();
let mut migrated = 0usize;
for mut account in retired {
account.provider = provider.clone();
match self.write_account(&account, CasExpectation::Any).await {
Ok(_) => migrated += 1,
Err(error) => {
tracing::debug!(
account_id = %account.id,
%error,
"slack provider migration: failed to rewrite account record"
);
}
}
}
migrated
}References
- When implementing backward compatibility logic, scope it precisely to the intended cases. Instead of hardcoding specific provider IDs, prefer checking for a common property (like a specific environment variable) that defines the group of providers needing the compatibility logic.
- Do not use
warn!orinfo!logging in REPL/TUI-reachable code as they can corrupt the REPL/TUI. Usedebug!logging instead, and ensure only sanitized identifiers are logged.
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | bfaf009834dc |
Head: bfaf009834dc9ad1e28756eff82be3190236ca2c
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found a blocking migration bug in the Slack extension identity merge. Bot-only legacy installations can be left with the retired installation id, which breaks later lifecycle operations for the unified Slack extension.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Use the unified installation id when renaming bot-only Slack installs
Location: crates/ironclaw_reborn_composition/src/extension_host/extension_installation_store.rs:289
When a legacy state has only the retired slack_bot installation and no existing slack installation, this branch creates an installation with extension_id = "slack" but keeps installation_id = "slack_bot". The lifecycle APIs derive the installation id from the package id (ExtensionInstallationId::new("slack")) in project, activate, and remove, so upgraded bot-only installs will appear in list output but cannot be projected/activated/removed as the unified Slack extension. The migration should create the renamed installation with the unified installation id (slack) and preserve the retired activation/bindings under that id; add a bot-only migration test because the current test only covers the case where a slack row already exists.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/stale/stalled state while reviewers run.
| } | ||
| } else if let Some(first) = retired.into_iter().next() | ||
| && let Ok(renamed) = ExtensionInstallation::new( | ||
| first.installation_id().clone(), |
There was a problem hiding this comment.
This preserves the retired slack_bot installation id even though the row is renamed to extension_id = "slack". Lifecycle lookups use ExtensionInstallationId::new("slack"), so a bot-only legacy install will be listed but later project/activate/remove for the unified Slack package won't find it. Please create the renamed row with the unified installation id and cover the bot-only migration case.
|
🚅 Deployed to the ironclaw-pr-5845 environment in ironclaw-ci-preview
|
bfaf009 to
de44310
Compare
10c8f36 to
6f820a6
Compare
9ba3bdf to
f542fc5
Compare
…ired
The Slack channel and the user-scoped Slack tools are one extension.
assets/slack/manifest.toml declares both surfaces: the
product_adapter.inbound channel section (Events API ingress, request
signature verification, host-authored bot egress, inbound+outbound
directions) and the capability_provider tools section (search, list,
history, user info, send-as-you), under provider `slack`. No per-surface
runtime was needed: the retired slack_bot manifest's first_party service
declaration was descriptive-only (the host mounts the channel service);
the wasm runtime serves the tools.
Deleted identities (no aliases): the slack_bot package, assets, digest,
catalog-hiding (is_internal_extension_package_ref), onboarding and
activation special cases; the slack_personal provider id everywhere
including the frontend OAuth-card display map ("personal" survives only
in flow-named identifiers for the user-scoped OAuth flow). The
slack_bot_token / slack_signing_secret credential HANDLES stay - they
are workspace secrets, not identities.
Two one-time forward data migrations, both pinned and idempotent:
- installation state: loading folds persisted slack_bot manifest records
and installation rows into the unified slack extension (enabled-wins
merge, credential bindings union, host-bundled manifest record seeded
when absent) and persists the migrated snapshot immediately;
- credential accounts: a boot sweep in the rooted factory builder
rewrites provider slack_personal -> slack via the durable account
store (sweep_all_accounts extracted from the refresh-candidate walk).
Operator setup-save activation uses the new
ChannelSetupActivationCredentialGate: per-caller product-auth accounts
never gate operator channel activation - each caller auth-gates at
tool-call time (auth_required). With the identities unified, the
per-caller channel-connection SetupRequired gate is live for the first
time: the connections key and the extension id finally match.
NEA-25 stack PR 4. Deployment note: Slack operator env referencing the
slack_personal provider id needs a one-word update.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7006a7b to
22cb072
Compare
…#5850) Atomic roll-up of the 8-PR NEA-25 taxonomy stack onto current main. Extension is the only installable product object; tool/channel/auth are derived capability surfaces; runtime kind controls loading only; manifest projection (v2, host_api contracts) is the sole surface-discovery source of truth. The connectable-channels rail and the parallel `kind` taxonomy are removed and pinned by a zero-legacy gate. slack_bot and slack_personal are retired into one `slack` extension with bounded forward migrations. Extensions wire carries runtime + surfaces, not a conflated kind. Supersedes #5833, #5839, #5842, #5845, #5847, #5848, #5849, #5850. Conflicts with main since the train forked were reconciled preserving main's newer behavior (#5851 unified slack cleanup, #6054 get_conversation_info DM resolution, #5499 extension import, #6057 TS source conventions). provider_identity domain duplication removed; the residual is a legitimate up-layer port adapter. See the PR description for the per-PR crosswalk, resolutions, placement audit, and verification. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Summary
Stack PR 4/7 for NEA-25, on #5842. Slack becomes the validation case for the unified model: one
slackextension, one manifest, both surfaces, providerslack.assets/slack/manifest.toml): theproduct_adapter.inboundchannel section (Events API ingress, request-signature verification, host-ingress route descriptor, bot egress — projectingchannel { inbound: true, outbound: true }) plus thecapability_provider.toolssection (the five user-scoped tools), under providerslack. No per-surface runtime schema was needed — the retiredslack_botmanifest'sfirst_partyservice declaration was descriptive-only (the host composes and mounts the channel service; the wasm runtime serves the tools), verified by consumer search before the merge.slack_botpackage/assets/digest/catalog-hiding (is_internal_extension_package_refdeleted crate-wide) and all lifecycle special cases;slack_personalprovider id everywhere including the frontend OAuth-card display map. "personal" survives only in flow-named identifiers for the user-scoped OAuth flow (SlackPersonalBindingService, theIRONCLAW_REBORN_SLACK_PERSONAL_OAUTH_REDIRECT_URIenv). Theslack_bot_token/slack_signing_secretcredential handles stay — workspace secrets, not identities.slack_botmanifest records + installation rows into the unifiedslackextension (enabled-wins merge, credential-binding union, host-bundled manifest record seeded when the tools package was never installed) and persists the migrated snapshot immediately —load_at_migrates_retired_slack_bot_identity_forward;slack_personal → slack(sweep_all_accountsextracted from the refresh-candidate walk so both consumers share one enumeration) —migrate_retired_slack_personal_provider_rewrites_accounts_forward.ChannelSetupActivationCredentialGate: operator setup-save activation no longer fails closed on per-caller OAuth accounts (they're user-scoped; each caller auth-gates at tool-call time viaauth_required). Previously the unified activation would have 503'd the setup route.SetupRequiredfor an unconnected caller was provably unreachable under the split identities (connections key"slack"vs channel packageslack_bot); with one identity the keys match and the mechanism works — pinned inunified_slack_extension_projects_surfaces_and_lists_auth_required_until_oauth_connected(surfaces:Tool + Channel{in,out} + Auth; state: SetupRequired until connected).Security invariants preserved: signature verification config unchanged (same adapter section), host-ingress descriptor identical, delegated-tool authority still rides the caller's OAuth account, bot egress still host-authored via the workspace token handle.
Testing
ironclaw_reborn_composition --all-features: 1,512 lib tests green (incl. the two migration pins, unified-manifest surface pins through the real bundled manifest, setup-save activation, onboarding copy)ironclaw_host_runtimegithub_wasm_runtime_contract (slack credential-injection + auth-gate contract) greenslacklegitimately appears in the authorize URL host, so the raw-id-never-renders invariant now checks standalone values)cargo check --workspace --all-targets --all-featuresclean; full clippy invocation cleanRootFilesystem, uniform across backends); live Slack E2E (needs workspace credentials — see deployment note)Deployment note: operator env/docs referencing the
slack_personalprovider id need a one-word update (~/shared-env/ironclaw/slack-test.envstyle setups); the OAuth redirect env var name is unchanged.🤖 Generated with Claude Code