feat(reborn): add host-owned ingress contracts - #3683
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a formal vocabulary for host-owned HTTP ingress within the ironclaw_host_api crate, effectively decoupling route declarations from server lifecycle management. It includes architectural tests to ensure product crates do not bind listeners directly and updates documentation to reflect these new boundaries. Feedback focuses on strengthening input validation for route patterns and justifications by explicitly rejecting path traversal segments and leading/trailing whitespace.
serrrfirat
left a comment
There was a problem hiding this comment.
Posted the review findings from the second pass.
serrrfirat
left a comment
There was a problem hiding this comment.
Security-focused follow-up review found two contract gaps around network security and multi-tenancy:
- Effectful routes can be declared with no tenant/user scope.
IngressPolicy::new validates auth/scope coherence and WebSocket origin, but it does not validate scope_source against effect_path (crates/ironclaw_host_api/src/ingress.rs). As a result, a descriptor with auth = Public, scope_source = PublicRoute, and effect_path = ProductWorkflow, TurnCoordinator, HostPort, or CapabilityHost is accepted.
For multi-tenant flows, anything that enters product workflow, turn coordination, host ports, or capability host needs a resolved tenant/user execution context. Otherwise host composition has to infer authority later, which is the kind of ambiguity this contract should reject up front. I recommend rejecting PublicRoute for effectful paths and allowing it only for NoEffect and, if intended, strictly read-only projection routes.
- Listener class does not require the matching network auth mechanism.
The contract defines security-sensitive listener classes and auth schemes, but validate_auth_scope only rejects generic public/authenticated scope mismatches. This means a PublicWebhook route can be declared as unauthenticated Public, and an InternalWorker route can omit InternalToken.
That conflicts with the repo's network model: webhook senders should prove a shared secret/signature, and worker/internal APIs should use scoped internal tokens. I recommend adding listener/auth coherence checks, for example: PublicWebhook requires WebhookSignature, InternalWorker requires InternalToken, local gateway user actions require bearer/session-style auth, and OAuth callbacks require OAuthState unless they are explicitly modeled as public no-effect callbacks.
0ae70e0 to
1cbcc0a
Compare
serrrfirat
left a comment
There was a problem hiding this comment.
Final pass review completed. No blocking findings; targeted local verification and current CI are green.
feat(reborn): add host-owned ingress contracts
Summary
ironclaw_host_apifor Reborn product/API surfaces.Change Type
Linked Issue
Closes #3578
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo buildcargo test -p ironclaw_host_api;cargo test -p ironclaw_architecturecargo test --features integrationif database-backed or integration behavior changedreview-prorpr-shepherd --fixwas run before requesting reviewAdditional targeted clippy:
cargo clippy -p ironclaw_host_api --all-targets -- -D warningscargo clippy -p ironclaw_architecture --all-targets -- -D warningsSecurity Impact
Yes. This adds security boundary contracts and tests for Reborn HTTP ingress. Product/API crates may describe routes and policies, but architecture tests guard against direct listener/server lifecycle ownership. No new listener, route mounting, auth implementation, network call, secret access, filesystem access, tool execution, or sandbox policy change lands in this PR.
Database Impact
None.
Blast Radius
Touches Reborn host API contract vocabulary, Reborn architecture boundary tests, and Reborn contract documentation. Existing v1 Web Gateway behavior is unchanged. Risk is mostly future-compatibility friction if a Reborn product/API crate intentionally needs host-owned route composition APIs; those should be exposed as descriptors or mounted by host composition rather than binding directly.
Rollback Plan
Revert this PR. It only adds contract types, docs, and architecture tests; no migrations or runtime behavior need rollback.
Review Follow-Through
Reviewer judgment requested on whether the initial ingress vocabulary is the right minimum shape before WebUI/WebChat/OpenAI-compatible consumer routes land. This PR intentionally does not mount routes, modify Web Gateway, or update
src/NETWORK_SECURITY.mdbecause no new Reborn listener lands here.Review track: B (feature/maintainer-requested refactor)