feat: manifest-driven channel ingress contract (ingress policy + auth + transport + credential coherence + generic serve) - #5107
Conversation
…rofile enum Replace the closed IngressPolicyProfile enum (a magic-string selector mapping "slack_events"/"telegram_updates" to a hardcoded Rust IngressPolicy) with an inline, typed [host_ingress.*.policy] declaration projected straight onto the existing IngressPolicy type. The route descriptor is built from the manifest's own route_id/method/path. Adding a channel's ingress is now manifest data, not a new enum variant + Rust policy function. Deleted from ironclaw_host_ingress_registry: - IngressPolicyProfile + all impls (from_manifest_name, as_str, route_descriptor, Display) - slack_events_policy, telegram_updates_policy, the two *_route_descriptor fns - all SLACK_EVENTS_* / TELEGRAM_UPDATES_* constants - the policy_profile field, ProfileRouteMismatch, and route-identity validation Auth honesty: add IngressAuthScheme::SharedSecretHeader so Telegram declares shared-secret-header auth and Slack stays webhook-signature, removing part of the lossy "everything is WebhookSignature" coercion. (The residual scheme-name -> scheme string match with a silent WebhookSignature fallback is carried to Move 2, where it becomes fail-closed.) Fail-closed: unknown enum value, zero limit, and missing field are rejected with typed errors (tests added). Both Slack and Telegram manifests migrated; projected policy is behavior-equal to the deleted Rust functions. Registry lib.rs shrank 968 -> 807 lines. Move 1 (keystone) of the manifest-driven-channels plan; see docs/plans/2026-06-20-manifest-driven-channels.md. Stacked on #5100. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Second half of the manifest-driven ingress contract, folded in with the IngressPolicy projection it builds on (was previously split as Move 2 / #5104). - Delete IngressAuthSchemeName + declared_auth_scheme(): they derived the auth verifier *kind* from the scheme *name* string with a silent `_ => WebhookSignature` fallback (stringly-typed and fail-OPEN in a security path, introduced by the policy-projection change above). IngressAuthBinding now carries a typed `verifier: IngressAuthScheme` declared directly in the manifest; unknown values reject at parse, and a verifier not in the route policy's allowed schemes fails closed. - Add a single-variant HostIngressTransport::Webhook discriminator (inline in lib.rs) wrapping the webhook route wiring, so policy/target/auth are the transport-agnostic envelope. No speculative websocket/polling variants — the enum is the seam for a future transport. - Route projection goes through project_host_ingress_section (no intermediate HostIngressSection wrapper); descriptor construction lives on the transport. - Fix the host_runtime host_api_contract_composition fixture to the new transport + verifier shape. Both manifests migrated; behavior-equal. Fail-closed tests added (unknown verifier, verifier-not-in-policy, unsupported transport kind). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… a re-derived one
The extension-projected mount builders iterated enabled-extension entries (each
carrying a manifest-projected route declaration) but then rebuilt the mount via
*_host_ingress_registrations(handler), re-deriving route metadata from Rust-side
helper constants — a second source of truth for the route. Use the projected
entry.declaration() directly via HostIngressRegistration { declaration, handler }
and gate the empty result on whether a projected declaration was found.
- Slack: keep slack_events_host_ingress_registrations — the explicit-config
single-installation path has no projected declaration in scope and still needs it.
- Telegram: remove telegram_updates_host_ingress_registrations (no remaining
caller); the lower-level telegram_updates_host_ingress_declaration stays for
its tests.
Behavior-preserving: the projected and re-derived declarations are equal today;
this removes the duplicate derivation so the mounted route comes from the manifest.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…generic plan serve.rs carried near-identical Slack and Telegram mount blocks (each with its own disabled/generic_shadow/generic mode logic) plus a connectable-channel cfg permutation matrix. Replace all of it with a single generic path: - New ironclaw_reborn_composition::host_ingress_serve_plan: build_host_ingress_serve _plan(&runtime, input) -> HostIngressServePlan, build_host_ingress_mounts_from _enabled_extensions, build_webui_services_with_host_ingress_plan. Mounts every enabled host-ingress route from its manifest-projected declaration; advertises connectable channels from manifest [[metadata.connectable.channels]] metadata. - serve.rs builds one HostIngressServePlanInput, lets the channel-owned serve_slack /serve_telegram helpers contribute projection policy, then applies the plan in a loop. No Slack-vs-Telegram mount block, no connectable cfg matrix. rg 'slack_events|telegram_updates|inbound_proof_code' serve.rs => no matches. - Per-channel #[cfg(feature)] is confined to composition + the channel-owned CLI helpers (per CLAUDE.md module-owned feature gating), not the mount path. Behavior preserved: all per-channel disabled/generic_shadow/generic semantics and the config-import mismatch errors are unchanged. Connectable channels still appear only when their route IDs are active. Per-channel config IMPORT is left for Move 5 (extension_setup contract) with a TODO(move-5) marker. Move 4 of the manifest-driven-channels plan. Stacked on #5103. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…projection Add a post-projection invariant in HostApiContractRegistry::project_manifest: every credential handle referenced by any host_api contract must resolve to a declared credential. Closes the "same credential spelled in two sections, drifts" class (bug #2574 family) at the single point where all contracts converge. - Canonical CredentialHandle (ironclaw_host_api::ids) carries credential identity across the per-domain newtypes; conversion happens only at the projection boundary. - HostApiManifestProjection gains declared_credentials + referenced_credentials (with host_api/section provenance); new ManifestV2Error::DanglingCredentialHandle. - Wired product-adapter (declared=required_credentials, referenced=egress handles) and capability-provider (referenced=SecretHandle runtime creds). - host-ingress reporting is the Move 1<->Move 3 merge seam (TODO in v2.rs). Move 3 of the manifest-driven-channels plan. Stacked on #5100. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ermo-nuclear cleanup)
Move 3 review pass. Hoist per-contract canonical CredentialHandle conversion +
ReferencedCredential provenance stamping out of each contract (and the test
fake) into HostApiManifestProjection::{declare,reference}_credential_handles, so
the boundary logic lives in one place instead of being copied at each call site.
Delete the now-thin ReferencedCredential::new wrapper. Behavior unchanged;
coherence pass still uses a single BTreeSet lookup and one empty-declared-set
decision point.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…o-nuclear round 2) Make HostApiManifestProjection's declared_credentials/referenced_credentials crate-private (capabilities stays public) so host-api contracts cannot bypass the canonical CredentialHandle conversion/provenance helpers and reintroduce duplicated boundary logic; drop the public ReferencedCredential re-export. Make declare/reference_credential_handles collect validated handles first and extend the projection only on success (no partial state on conversion error). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tor shape Integration fix surfaced by combining all four moves into one tree. The fake host_ingress contract in manifest_ingestion.rs validated route_id at the section top level, but Move 2 moved route_id under [host_ingress.*.transport]. The stub now reads transport.route_id, matching the real manifest shape. Pure test-double fix; no production change. Co-Authored-By: Claude Opus 4.8 <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:
Comment |
There was a problem hiding this comment.
Code Review
This pull request implements the first phase of the manifest-driven channels plan, replacing the hardcoded IngressPolicyProfile enum with inline, typed policy declarations in extension manifests. It also introduces a canonical CredentialHandle type and a post-projection validation step to ensure cross-contract credential coherence. Additionally, the PR refactors the host ingress serve planning to generically project and mount transports, significantly reducing per-channel configuration boilerplate. Feedback on the changes suggests avoiding redundant TOML parsing of the manifest when projecting connectable channels by parsing and validating this metadata exactly once during extension loading.
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 document: toml::Value = toml::from_str(manifest.raw_toml()).map_err(|error| { | ||
| HostIngressServePlanError::InvalidConnectableMetadata { | ||
| extension_id: manifest.extension_id().to_string(), | ||
| reason: error.to_string(), | ||
| } | ||
| })?; |
There was a problem hiding this comment.
Parsing the TOML manifest here via toml::from_str(manifest.raw_toml()) is redundant because the manifest has already been parsed and validated during extension loading. Consider projecting the connectable channels metadata exactly once during manifest parsing (e.g., by adding a connectable_channels field to ExtensionManifestV2 or projecting it via a host API contract) and replaying the pre-parsed entries here to avoid redundant TOML parsing at runtime.
References
- Avoid redundant parsing and iteration of configuration files (e.g., TOML manifests) by projecting them into typed entries exactly once, validating the set, and replaying the pre-parsed entries.
Review — closing as supersededReviewed as part of a parallel pass over the earliest open Reborn ingress stack (#5072 → #5093 → #5100 → #5107). This is the top of the stack and its pivotal PR. Verdict: REJECT-SUPERSEDED (partial) → closing. It makes channel ingress/auth/transport/secrets manifest-defined via a Supersession (verified against live main):
Findings:
Maintainability verdict: re-derive fresh, don't land the stack. The declarative/typed/fail-closed ingress-policy contract already exists on main via #3683, more compactly. The genuinely-novel manifest-projection + serve-plan idea is worth having but is cheaper to re-express against 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
Makes channel/extension ingress, auth, transport, secrets, and connect onboarding manifest-defined instead of provider-specific Rust. This is the consolidation of what were four stacked/sibling PRs (#5103, #5102, #5106 — now closed in favor of this) into one self-contained, integration-tested branch, so it can be reviewed and verified as a unit.
What's in it (8 commits)
IngressPolicyfrom the manifest; delete the profile enum — replaces the closedIngressPolicyProfile(magic-string → hardcoded Rust policy) with an inline[host_ingress.*.policy]declaration projected onto the typedIngressPolicy.IngressAuthSchemeName/declared_auth_scheme()(a fail-open_ => WebhookSignaturestring fallback);IngressAuthBindingnow carries a typedverifier: IngressAuthScheme. Adds a single-variantHostIngressTransport::Webhookso policy/target/auth are the transport-agnostic envelope.#[cfg]sprawl into one data-drivenHostIngressServePlan. Adding a channel touches zero lines in serve.rs; connectable advertisement comes from manifest[[metadata.connectable.channels]].CredentialHandle; every credential referenced by any host_api contract must resolve to a declared one (DanglingCredentialHandle, fail-closed), closing the bug-fix: resolve staging CI test failures blocking promotion #2574 drift class at the projection choke point.Why one PR
The four were independently green but had never been built together. Consolidating immediately caught a real cross-move break: Move 2 moved
route_idunder[host_ingress.*.transport], while a credential-coherence test stub still expected it top-level. Pairwise testing couldn't catch it. Fixed (commit 8). This is the value of a self-contained branch.Verification (combined tree, on host)
cargo fmtclippy -D warningsacross all 7 touched crates → cleancargo test(extensions, product_adapter_registry, host_api, host_ingress_registry) → passhost_runtimehost_api_contract_composition fixture → pass (4/4)webui-v2-beta,slack-v2-host-betawebui-v2-beta,telegram-v2-host-betawebui-v2-beta,slack-v2-host-beta,telegram-v2-host-betawebui-v2-betaBehavior
Behavior-preserving throughout: manifests migrated, projected declarations byte-equal, all per-channel
disabled/generic_shadow/genericsemantics + config-import mismatch errors unchanged. Two thermo-nuclear review passes applied (wrapper deletions, duplicate-check removal, transport inlining, projection encapsulation/atomicity).Deferred (Move 5, not in this PR)
Per-channel config import and the two onboarding contracts (
extension_setup/v1,connectable_channel/v1) are scoped but deferred — markedTODO(move-5). Decision recorded:extension_setupwill be the single declarer of channel secrets (everyone else references by handle). The Move 1↔Move 3 host-ingress credential-reporting seam is the small remaining wire-up.Stacking
Base is
stack/5100-telegram-ingress(snapshot of #5100's tip); diff is only this work. Stacks under the unmerged #5072 → #5093 → #5100 chain; retarget tomainonce that lands. Supersedes #5103, #5102, #5106.🤖 Generated with Claude Code