feat(reborn): project Slack ingress from extension state - #5093
serrrfirat wants to merge 1 commit into
Conversation
|
🚅 Deployed to the ironclaw-pr-5093 environment in ironclaw-ci-preview
|
|
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:
Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces support for projecting Slack events webhook routes from enabled extension states in generic host-ingress mode. It adds the ironclaw_host_ingress_registry contract, implements a filesystem-backed store for Slack extension settings, and integrates the secret store to dynamically resolve credentials. The feedback suggests optimizing performance by using serde_json::to_vec instead of serde_json::to_vec_pretty for settings serialization, and passing the secret store by reference rather than cloning its Arc in helper functions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let body = | ||
| serde_json::to_vec_pretty(&StoredSlackExtensionInstallationSettings::from(settings)) | ||
| .map_err(|error| Error::Invalid { | ||
| reason: format!("Slack extension settings could not be serialized: {error}"), | ||
| })?; |
There was a problem hiding this comment.
Using serde_json::to_vec_pretty adds unnecessary whitespace and formatting overhead for settings that are stored and read programmatically. Using serde_json::to_vec is more efficient in terms of both serialization performance and storage footprint.
| let body = | |
| serde_json::to_vec_pretty(&StoredSlackExtensionInstallationSettings::from(settings)) | |
| .map_err(|error| Error::Invalid { | |
| reason: format!("Slack extension settings could not be serialized: {error}"), | |
| })?; | |
| let body = | |
| serde_json::to_vec(&StoredSlackExtensionInstallationSettings::from(settings)) | |
| .map_err(|error| Error::Invalid { | |
| reason: format!("Slack extension settings could not be serialized: {error}"), | |
| })?; |
| async fn read_secret_string( | ||
| secret_store: Arc<dyn SecretStore>, | ||
| scope: &ResourceScope, | ||
| handle: &SecretHandle, | ||
| ) -> Result<SecretString, SlackHostBetaBuildError> { | ||
| let lease = secret_store.lease_once(scope, handle).await?; | ||
| Ok(secret_store.consume(scope, lease.id).await?) | ||
| } |
There was a problem hiding this comment.
The read_secret_string helper function takes secret_store by value as Arc<dyn SecretStore>. Since it only needs to call methods on the store, passing it by reference as &dyn SecretStore is more idiomatic and avoids unnecessary Arc cloning.
| async fn read_secret_string( | |
| secret_store: Arc<dyn SecretStore>, | |
| scope: &ResourceScope, | |
| handle: &SecretHandle, | |
| ) -> Result<SecretString, SlackHostBetaBuildError> { | |
| let lease = secret_store.lease_once(scope, handle).await?; | |
| Ok(secret_store.consume(scope, lease.id).await?) | |
| } | |
| async fn read_secret_string( | |
| secret_store: &dyn SecretStore, | |
| scope: &ResourceScope, | |
| handle: &SecretHandle, | |
| ) -> Result<SecretString, SlackHostBetaBuildError> { | |
| let lease = secret_store.lease_once(scope, handle).await?; | |
| Ok(secret_store.consume(scope, lease.id).await?) | |
| } |
| let bot_token = | ||
| read_secret_string(Arc::clone(&secret_store), &secret_scope, &bot_secret_handle) | ||
| .await?; |
There was a problem hiding this comment.
If read_secret_string is updated to take &dyn SecretStore by reference, we can avoid cloning the Arc here by passing secret_store.as_ref() directly.
| let bot_token = | |
| read_secret_string(Arc::clone(&secret_store), &secret_scope, &bot_secret_handle) | |
| .await?; | |
| let bot_token = | |
| read_secret_string(secret_store.as_ref(), &secret_scope, &bot_secret_handle) | |
| .await?; |
Review — closing as supersededReviewed as part of a parallel pass over the earliest open Reborn ingress stack (#5072 → #5093 → #5100 → #5107). Verdict: REQUEST CHANGES → closing. The mechanism (project the Slack Events route from enabled extension installation state, secret-store-backed signing/bot credentials, legacy-config→extension import bridge) is genuinely novel — but its entire substrate is unlanded and main took a competing design. Supersession / blocking facts (verified against live main):
Substantive findings (worth carrying forward even though closing):
Closing in favor of a single fresh PR that re-derives manifest-projected ingress atop main's current |
|
Closing as superseded by main's independently-evolved ingress design (ironclaw_host_api::ingress from #3683 + the product_adapters/telegram_v2_adapter stack). See the review comment above. The one still-valuable idea — manifest-projected ingress descriptor + generic serve plan — is being re-derived fresh atop current main in a replacement PR rather than rebasing this ~250-commit-stale, 4-deep stack. |
…dential coherence (#5625) * feat(reborn): manifest-projected host-ingress route + fail-closed credential coherence Re-derives the one still-valuable idea from the superseded earliest-reborn ingress stack (#5072/#5093/#5100/#5107, closed) fresh atop main's current `ironclaw_host_api::ingress` contract — as a single small, self-contained registry-only change, instead of rebasing a ~250-commit-stale 4-deep stack built on a `host_ingress_registry` crate main never adopted. A ProductAdapter manifest section may now declare `[[...host_ingress]]` routes, each carrying a full host-owned `IngressRouteDescriptor` (validated by host_api's own Deserialize — dotted route id, absolute path, and every policy invariant including the fail-closed floor that a `public_webhook` listener MUST require `webhook_signature`) plus the `credential_handles` that verify it. The registry does NOT re-own ingress route/policy vocabulary; it projects the descriptor and adds the binding host_api deliberately lacks — ingress credential coherence, enforced fail-closed: - every credential handle must be declared in `required_credentials` (mirrors the egress rule; ingress handles flow into the same declared set installation bindings validate against), - an auth-required route must name at least one verifying credential handle (no route nothing can authenticate), - route ids stay distinct within a section. This is exactly the seam PR #5107's review flagged as fail-open/TODO'd. The serve-layer generic mount (descriptor -> axum route + reused SharedSecretHeader/Hmac verifier) is the deliberate follow-up; main's PublicRouteMount + descriptor-fold middleware already generalize, so it is a small wiring change with no new crate. Tests: ingress credential-coherence matrix (undeclared handle, auth-required route missing credential, duplicate route id, happy projection) as focused unit tests in src/lib.rs; wire-path projection, the inherited fail-closed floor, and coherence-over-the-wire as integration tests in tests/manifest_ingestion.rs. fmt + clippy (all-features, --tests) clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review: reject public host-ingress routes that declare credentials; robustness + type nits Addresses bot review feedback on #5625: - gemini (security): a public (no-auth) host-ingress route declaring a credential handle is incoherent and misleading (a reader would assume it is authenticated). Add PublicIngressRouteHasCredential and reject it, making the coherence rule symmetric with the auth-required-needs-credential rule. Adds two unit tests (rejection + the valid no-credential complement). - gemini (types): store &IngressRouteId in the route-id dedup set instead of downgrading to &str. - copilot/coderabbit (tests): the fail-closed-floor wire test now accepts both RegistryError::Manifest(_) and ManifestSectionParse, so it pins the fail-closed behavior rather than the error-routing path. - copilot (naming): documented on HostIngressRoute::credential_handles why the shared EgressCredentialHandle newtype is reused for ingress (type-placement rule) and that its Display leaks no "egress" wording. cargo fmt + clippy (all-features, --tests) clean; registry tests green (6 unit incl. the 2 new public-route cases + 9 manifest + 13 contract). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…the Rust policy literals (#5626) * feat(reborn): manifest-projected host-ingress route + fail-closed credential coherence Re-derives the one still-valuable idea from the superseded earliest-reborn ingress stack (#5072/#5093/#5100/#5107, closed) fresh atop main's current `ironclaw_host_api::ingress` contract — as a single small, self-contained registry-only change, instead of rebasing a ~250-commit-stale 4-deep stack built on a `host_ingress_registry` crate main never adopted. A ProductAdapter manifest section may now declare `[[...host_ingress]]` routes, each carrying a full host-owned `IngressRouteDescriptor` (validated by host_api's own Deserialize — dotted route id, absolute path, and every policy invariant including the fail-closed floor that a `public_webhook` listener MUST require `webhook_signature`) plus the `credential_handles` that verify it. The registry does NOT re-own ingress route/policy vocabulary; it projects the descriptor and adds the binding host_api deliberately lacks — ingress credential coherence, enforced fail-closed: - every credential handle must be declared in `required_credentials` (mirrors the egress rule; ingress handles flow into the same declared set installation bindings validate against), - an auth-required route must name at least one verifying credential handle (no route nothing can authenticate), - route ids stay distinct within a section. This is exactly the seam PR #5107's review flagged as fail-open/TODO'd. The serve-layer generic mount (descriptor -> axum route + reused SharedSecretHeader/Hmac verifier) is the deliberate follow-up; main's PublicRouteMount + descriptor-fold middleware already generalize, so it is a small wiring change with no new crate. Tests: ingress credential-coherence matrix (undeclared handle, auth-required route missing credential, duplicate route id, happy projection) as focused unit tests in src/lib.rs; wire-path projection, the inherited fail-closed floor, and coherence-over-the-wire as integration tests in tests/manifest_ingestion.rs. fmt + clippy (all-features, --tests) clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review: reject public host-ingress routes that declare credentials; robustness + type nits Addresses bot review feedback on #5625: - gemini (security): a public (no-auth) host-ingress route declaring a credential handle is incoherent and misleading (a reader would assume it is authenticated). Add PublicIngressRouteHasCredential and reject it, making the coherence rule symmetric with the auth-required-needs-credential rule. Adds two unit tests (rejection + the valid no-credential complement). - gemini (types): store &IngressRouteId in the route-id dedup set instead of downgrading to &str. - copilot/coderabbit (tests): the fail-closed-floor wire test now accepts both RegistryError::Manifest(_) and ManifestSectionParse, so it pins the fail-closed behavior rather than the error-routing path. - copilot (naming): documented on HostIngressRoute::credential_handles why the shared EgressCredentialHandle newtype is reused for ingress (type-placement rule) and that its Display leaks no "egress" wording. cargo fmt + clippy (all-features, --tests) clean; registry tests green (6 unit incl. the 2 new public-route cases + 9 manifest + 13 contract). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(reborn): project Slack ingress routes from the manifest, delete the Rust policy literals Makes the manifest-driven ingress contract from #5625 load-bearing: Slack's two inbound routes are now declared as data in the bundled extension manifest and projected into descriptors at serve time, instead of being hand-written Rust policy literals. This is the real example of usage the mechanism needed. - assets/slack/manifest.toml declares `[[product_adapter.inbound.host_ingress]]` for `slack.events` and `slack.commands`, each naming `slack_bot_token` as its verifying credential (fail-closed credential coherence, enforced by the registry). - slack_serve::slack_events_route_descriptors / slack_commands_route_descriptors now project their descriptor from the bundled manifest via a new generic helper (composition::host_ingress::bundled_host_ingress_descriptor), and the two hardcoded slack_events_policy() / slack_commands_policy() literals are deleted. - Only the declarative descriptor moved to the manifest. The axum handler and the HMAC verifier (behavior) stay in Rust — a manifest cannot carry behavior. The descriptor is validated by ironclaw_host_api on deserialize (dotted route id, absolute path, and the fail-closed floor that a public_webhook listener MUST require webhook_signature) and by ironclaw_product_adapter_registry for ingress credential coherence — so a manifest cannot declare a weaker route than the Rust literal did. Stacked on #5625 (needs ProductAdapterHostApiSection::host_ingress()). This also answers the "unused API" question on #5625: the accessor now has a production consumer that deletes per-channel Rust. Tests: two behavior-preserving equivalence guards assert the manifest-projected descriptor is byte-for-byte identical to the pre-migration literal (slack_{events,commands}_route_descriptor_matches_manifest_projection); the existing 377 Slack serve/e2e/handler tests still drive real signed and forged webhooks through the projected mounts (all pass). fmt + clippy (slack-v2-host-beta,webui-v2-beta,libsql; --tests) clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review: cache the Slack ingress descriptor projection with LazyLock Addresses gemini review feedback on #5626: project each Slack route descriptor from the bundled manifest exactly once (LazyLock) instead of re-parsing the manifest TOML on every slack_{events,commands}_route_descriptors() call. The manifest is a compile-time constant, so the projection is deterministic and safe to memoize for the process lifetime. Behavior-preserving: the equivalence guards and the full 377-test Slack serve suite still pass; clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review: signing-secret verifier handle, descriptor-driven mounts, single manifest parse path Address PR #5626 review feedback: - manifest.toml: the ingress routes' credential_handles now name slack_signing_secret (declared in required_credentials) — the secret the runtime's HMAC webhook verifier is actually built from — instead of the outbound slack_bot_token. Descriptors expanded from one-line inline tables into multiline TOML tables so policy changes diff field-by-field. - slack_serve: both descriptors project from one manifest parse via a single LazyLock, and the axum mounts now build their route path from the projected descriptor (failing closed at projection time if a route ever declares a non-POST method), so what axum mounts cannot drift from what the manifest declares. - host_ingress: parsing reuses the same context as bundled extension installation (default host-port catalog + default host-API contract registry) instead of a second ingestion path with an empty catalog; projects all routes in one pass with a separate route-id selector. - host_ingress tests: cover the happy projection and the previously untested RouteNotDeclared branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Stacked on #5072.
Tests