Skip to content

feat(reborn): add product adapter host auth and egress primitives - #3352

Merged
nickpismenkov merged 11 commits into
reborn-integrationfrom
split/pr3316-02-product-adapter-host-auth-egress
May 12, 2026
Merged

nickpismenkov merged 11 commits into
reborn-integrationfrom
split/pr3316-02-product-adapter-host-auth-egress

Conversation

@serrrfirat

@serrrfirat serrrfirat commented May 7, 2026 •

Copy link
Copy Markdown
Collaborator

Split from #3316 (original PR by @nickpismenkov). This is PR 2/7 in the ProductAdapter stack.

Summary

  • Add ironclaw_wasm_product_adapters host auth/egress primitive crate.
  • Add constant-time HMAC/shared-secret webhook auth verifiers.
  • Add declared-host + credential-handle egress policy.
  • Add initial wit/product_adapter.wit shape.
  • Intentionally excludes runner glue and Telegram behavior.

Stack

Authorship

Split commits retain Co-authored-by: Nikolay Pismenkov <nickpismenkov@gmail.com>.

Verification

  • cargo test -p ironclaw_wasm_product_adapters --offline --quiet — 13 tests passed on this slice.
  • Final stack also ran cargo fmt --all -- --check and targeted ProductAdapter/Telegram/config tests.

@github-actions github-actions Bot added scope: dependencies Dependency updates size: XL 500+ changed lines risk: medium Business logic, config, or moderate-risk modules contributor: core 20+ merged PRs labels May 7, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the ironclaw_wasm_product_adapters crate, which establishes the host runtime primitives for WASM v2 product adapters, focusing on protocol authentication and egress policy enforcement. Key components include HMAC and shared-secret verifiers, an egress allow-list mechanism, and the WIT contract defining the component model boundary. The review feedback identifies several improvement opportunities: enhancing the robustness of system clock handling by avoiding default values, improving type safety for timestamp parsing through newtypes, refining error handling with specific variants and structured logging, and utilizing a standard crate for hex encoding instead of a custom implementation.

Comment on lines +49 to +50
.unwrap_or(0)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using unwrap_or(0) for duration_since(UNIX_EPOCH) can mask potential issues if the system clock is set to a time before the Unix epoch. Following repository rules, if a timestamp is unknown or invalid, treat it as un-reapable by logging a warning and skipping the operation. Do not fall back to a default value like 0, as this can cause the resource to perpetually appear new and prevent proper cleanup or validation.

References
  1. When calculating resource age or timing, if the timestamp is unknown, log a warning and skip it instead of falling back to a default value.

// Replay-window check before computing HMAC. Reject stale or
// far-future timestamps; both are forgery attempts. The window is
// symmetric: |now - ts| > max_age_secs => fail.
let Ok(timestamp_secs) = timestamp_str.parse::<i64>() else {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The now_unix_seconds method returns u64, but timestamp_str.parse::<i64>() parses the incoming timestamp as i64. To align with the 'Parse, Don't Validate' approach, consider using a newtype wrapper or parsing directly into the target type (u64) at the boundary to ensure type safety and consistency.

References
  1. Prefer creating a newtype wrapper that validates on construction to ensure type safety and centralize validation at system boundaries.

Comment on lines +167 to +172
let signed_payload = format!("v0:{timestamp_str}:");
let Ok(mut mac) = Hmac::<Sha256>::new_from_slice(&self.signing_secret) else {
// HMAC-SHA-256 accepts arbitrary key lengths in the algorithm
// spec — `new_from_slice` should never reject a non-empty key.
// Treat the unexpected error as a malformed configuration.
return VerificationOutcome::Failed {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error handling for Hmac::<Sha256>::new_from_slice currently treats any unexpected error as ProtocolAuthFailure::Malformed. Per repository rules, create specific error variants for different failure modes. Additionally, ensure detailed errors from external dependencies are logged using a structured logger (like tracing) while returning a sanitized error to the caller.

References
  1. Create specific error variants for different failure modes to provide semantically correct and clear error messages.
  2. Log detailed errors from external dependencies using structured logging while returning sanitized errors to callers.

Comment on lines +220 to +227
mod hex {
pub fn encode(bytes: impl AsRef<[u8]>) -> String {
let mut out = String::with_capacity(bytes.as_ref().len() * 2);
for byte in bytes.as_ref() {
out.push_str(&format!("{byte:02x}"));
}
out
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This module implements a custom hex::encode function. For consistency, security, and maintainability, it's generally recommended to use a well-vetted external crate like the hex crate. Re-implementing such utility functions, especially in a security-sensitive context, can introduce subtle bugs.

pub use hex::encode;

subtle = "2"
thiserror = "2"
tracing = "0.1"
uuid = { version = "1", features = ["v4", "serde"] }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To use the hex::encode function from the hex crate as suggested in auth_verifier.rs, you need to add hex = "0.4" to the [dependencies] section of this Cargo.toml file.

Suggested change
uuid = { version = "1", features = ["v4", "serde"] }
uuid = { version = "1", features = ["v4", "serde"] }
hex = "0.4"

@serrrfirat
serrrfirat force-pushed the split/pr3316-01-product-adapter-contract branch 2 times, most recently from de3a82d to f8b4415 Compare May 7, 2026 21:25
Base automatically changed from split/pr3316-01-product-adapter-contract to reborn-integration May 7, 2026 21:25
@serrrfirat
serrrfirat force-pushed the split/pr3316-02-product-adapter-host-auth-egress branch 2 times, most recently from 345bca4 to 26d1e8a Compare May 8, 2026 14:29
@github-actions github-actions Bot added the scope: docs Documentation label May 8, 2026
Split from PR #3316. Lands constant-time webhook auth verification, declared-host/credential-handle egress policy, and the WIT contract before adding runner glue or Telegram behavior.

Co-authored-by: Nikolay Pismenkov <nickpismenkov@gmail.com>
@serrrfirat
serrrfirat force-pushed the split/pr3316-02-product-adapter-host-auth-egress branch from 26d1e8a to 296f370 Compare May 8, 2026 14:30
@ilblackdragon

Copy link
Copy Markdown
Member

Code Review: PR #3352 — feat(reborn): add product adapter host auth and egress primitives

Where this fits in the Reborn integration

This PR adds the WASM v2 product-adapter trust boundary — webhook auth verification and constrained egress — for the upcoming WASM-based product adapters (Slack, Telegram, etc., as components rather than native Rust). Recall the broader stack:

Webhook delivery (Slack/Telegram/Discord)
  ↓ host glue
[1] WebhookAuthVerifier (THIS PR)              ← HMAC / shared-secret check
  ↓ verified → mint sealed VerifiedAuthClaim (host_only seal in ironclaw_product_adapters)
WASM ProductAdapter component (future PR)
  ↓ parse-inbound(raw_payload, evidence) → ParsedProductInbound
host glue stamps ProductInboundEnvelope
  ↓
DefaultProductWorkflow (#3428)
  ↓ TurnCoordinator → scheduler (#3438) → host (#3439) → driver → exit (#3446)

The trust seam: WASM cannot mint Verified evidence — the seal type is unreachable inside the component. The host minting is gated by feature flags (host-auth-mint, test-support).

The WIT file in this PR documents the eventual component contract; the bindgen wiring lands in a later PR ("the WIT below is the agreed shape for the eventual component build").

Strengths

  • Constant-time comparison everywhere. Both HmacWebhookAuth and SharedSecretHeaderAuth use subtle::ConstantTimeEq. No timing oracles. ✅
  • Symmetric replay window. HMAC verifier rejects timestamps drifted in either direction ((now_secs - timestamp_secs).abs() > max_age) — both stale captures and far-future forgeries fail. Default 5 min matches Slack convention.
  • Clock seam. Clock trait + SystemClock + FixedClock makes time-window tests deterministic. The verifier_at(now_secs, max_age_secs, secret) test helper produces clean timestamp-window tests.
  • Architecture boundary tests are exemplary. Three of them:
    • wasm_product_adapter_crate_has_local_guardrails — CLAUDE.md must exist.
    • wasm_product_adapter_crate_keeps_minimal_host_glue_dependencies — pins the exact dep list [hmac, http, ironclaw_product_adapters, sha2, subtle, thiserror]. Adding any new dep requires explicit test update.
    • wasm_product_adapter_wit_preserves_product_adapter_trust_boundary — greps the WIT file for required record names AND forbidden ones (record parsed-envelope, envelope-json, Returns \none`, ProductInboundEnvelope`). Locks the trust contract at the WIT layer. Excellent regression scaffolding.
  • EgressPolicy is fail-closed. Undeclared host → error; unknown credential handle → error. Tests cover both paths. The "declared host with known handle passes" test confirms the success path.
  • WIT design is conservative. Egress responses don't expose raw headers (!response_record.contains("headers") test). host-index: u32 and credential-handle-index: option<u32> mean the component only sees indices into a host-controlled allowlist — no raw URLs. log and now-millis are the only general-purpose imports — no filesystem, no env, no random.

Issues

Typed-internals (.claude/rules/types.md)

  • EgressPolicy stores BTreeSet<String> instead of BTreeSet<DeclaredEgressHost> / BTreeSet<EgressCredentialHandle> (egress_policy.rs:773-775). Constructor takes typed values and immediately drops to String via .as_str().to_string(). Same anti-pattern as in feat(reborn): add text-only loop driver host factory #3439 / feat(reborn): add ProductWorkflow and InboundTurnService facade (#3280) #3428 — typed values are validated, then thrown away inside the policy. Storing the typed values keeps the validated invariant alive across lookups and would let declared_hosts() return impl Iterator<Item = &DeclaredEgressHost> instead of &str. Fix:
    pub struct EgressPolicy {
        declared_hosts: BTreeSet<DeclaredEgressHost>,
        allowed_credential_handles: BTreeSet<EgressCredentialHandle>,
    }
  • EgressPolicyError::UndeclaredHost { host: String } / UnauthorizedCredentialHandle { handle: String }. Errors are at the boundary so this is forgivable (they go to logs/wire), but the typed values exist — carrying them in the error struct preserves type identity through error propagation. Lower priority than the storage fix above.

Correctness

  • HmacWebhookAuth::Hmac::<Sha256>::new_from_slice(&self.signing_secret) only fails on empty key. The comment says "should never reject a non-empty key" — true. But empty keys would hit this path and return ProtocolAuthFailure::Malformed. Add a test for the empty-key case to lock the behavior; an empty signing secret is a configuration bug worth detecting at verify time.
  • The hand-rolled hex module uses format!("{byte:02x}") per byte — heap allocation per byte. For 32-byte HMAC digests it's 32 allocs per verify. The hex crate is ~50 LOC and well-audited; consider depending on it (the dep-list test would need updating). Negligible perf at low rates; matters at high webhook volume.
  • auth.rs adds 6 #[cfg_attr(not(...), allow(dead_code, reason = "..."))] annotations. A lot of repetition. Either factor into a single module-level #![cfg_attr(...)] if scoping permits, or leave as-is — the reason = "..." field is informative and worth keeping. Note: reason requires Rust 1.81+; verify MSRV.

WIT contract

  • evidence: auth-evidence in parse-inbound is just record auth-evidence { evidence-json: string } in WIT — i.e., an opaque JSON string. The architecture comment says "Components cannot mint a verified auth-evidence value" but the WIT doesn't enforce that — the seal lives in the host-side Deserialize impl in auth.rs that rejects verified variants from untrusted JSON. This is correct but invisible to a WIT reader. Add a comment in the WIT block explaining: "// The host serializes only sealed evidence into evidence-json; component-supplied JSON cannot impersonate a verified claim because the host's Deserialize impl rejects unsealed verified variants."

Test coverage

  • No test for EgressPolicy round-trip with multiple hosts/handles. Three tests cover single-host, single-handle. A test with two declared hosts where one matches and one doesn't would lock the iteration semantics.
  • No test that HmacWebhookAuth::with_max_age(0) rejects all timestamps. Edge case but the symmetric drift check (drift > max_age) means any nonzero drift fails when max_age == 0. A "max_age=0 always rejects" test makes the symmetry explicit.

Risk

risk: medium is right. The crypto is constant-time, the boundary tests are tight, the WIT is conservative. The typed-internals issues are the only blocking items per the rule.

Summary

Approve with these:

  1. Store BTreeSet<DeclaredEgressHost> / BTreeSet<EgressCredentialHandle> in EgressPolicy instead of BTreeSet<String> per .claude/rules/types.md.
  2. Add a comment in the WIT explaining that the auth-evidence seal lives in the host-side Deserialize impl — the WIT alone doesn't enforce it.
  3. Add empty-signing-secret test for HmacWebhookAuth to lock the malformed-config path.
  4. Verify Rust MSRV supports reason = "..." in cfg_attr(allow(...)) (1.81+).

The architecture and crypto discipline are excellent. Typed-internals is the cleanup before merge; everything else is polish.

@serrrfirat

Copy link
Copy Markdown
Collaborator Author

Addressed ilblackdragon review feedback in fbe71d9e:

  • Kept EgressPolicy internals typed: BTreeSet<DeclaredEgressHost> and BTreeSet<EgressCredentialHandle>; policy errors now carry typed values too.
  • Updated policy iterators to return typed refs and added multi-host/multi-handle coverage for membership + fail-closed paths.
  • Added WIT comment explaining auth-evidence.evidence-json is host-serialized sealed evidence and component JSON cannot impersonate verified claims because host-side deserialize rejects unsealed verified variants.
  • Added HMAC empty signing-secret coverage; empty secret now fails as malformed config before digest work.
  • Added with_max_age(0) drift coverage for the replay-window edge case.
  • Removed per-byte format! allocation in the local hex encoder without adding a dependency, preserving the minimal dependency guardrail.
  • Verified allow(reason = ...) is supported by current MSRV: workspace/crates use rust-version = "1.92".

Local verification:

  • cargo test -p ironclaw_wasm_product_adapters
  • cargo test -p ironclaw_product_adapters
  • cargo test -p ironclaw_architecture wasm_product_adapter
  • cargo fmt --check
  • cargo clippy -p ironclaw_wasm_product_adapters -p ironclaw_product_adapters -p ironclaw_architecture --tests -- -D warnings
  • bash scripts/pre-commit-safety.sh
  • git diff --check

@serrrfirat

Copy link
Copy Markdown
Collaborator Author

Follow-up complete after the strategy audit.

Updates pushed in d433e52b:

  • Merged current reborn-integration into this PR and resolved the workspace-member conflict by preserving the new Reborn crates plus crates/ironclaw_wasm_product_adapters.
  • Added an explicit zero-window HMAC test: with_max_age(0) accepts an exact timestamp and rejects any nonzero drift.
  • Re-ran local gates against the updated base:
    • cargo test -p ironclaw_wasm_product_adapters
    • cargo test -p ironclaw_product_adapters
    • cargo test -p ironclaw_architecture wasm_product_adapter
    • cargo fmt --check
    • targeted clippy with -D warnings
    • scripts/pre-commit-safety.sh against origin/reborn-integration
    • git diff --check origin/reborn-integration...HEAD
  • GitHub checks on current head are green: Code Style, Clippy, Regression enforcement, Reborn CLI smoke, cargo-deny, no-panics, gateway checks, classify/scope.

@ilblackdragon ready for re-review.

Conflict was in `Cargo.toml`'s workspace `members`:

- #3352 adds `crates/ironclaw_wasm_product_adapters`
- `reborn-integration` adds `crates/ironclaw_storage` and
  `crates/ironclaw_reborn_config`

Resolved as the union — all three crates are listed. The directories
for all three exist on disk after the auto-merge brought in the
sibling-side files. All other files merged cleanly.

Verified:
- `cargo check --workspace` clean
- `cargo fmt --all -- --check` clean
- `cargo clippy --workspace --all-targets -- -D warnings` zero warnings
- `cargo test -p ironclaw_architecture` (boundary tests) green
- `cargo test -p ironclaw_product_adapters` green

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nickpismenkov added a commit that referenced this pull request May 12, 2026
Merges split/pr3316-02-product-adapter-host-auth-egress (which itself
just synced with reborn-integration) into this branch. Most of the
diff is the upstream reborn-integration content #3353 hadn't seen yet.

## Conflict resolution

22 paths conflicted. Resolution rules applied:

- `Cargo.toml` (workspace members) — took #3352's post-sync version.
  No #3353-unique workspace members.
- `Cargo.lock` — took #3352's post-sync version; cargo settled it
  against the resolved Cargo.tomls.
- `crates/ironclaw_product_adapters/**` (15 files: CLAUDE.md, Cargo.toml,
  src/{adapter,auth,egress,error,external,fakes,identity,inbound,lib,
  outbound,projection,workflow}.rs, tests/product_adapter_contract.rs)
  AND `crates/ironclaw_wasm_product_adapters/wit/product_adapter.wit`
  — took #3352's post-sync versions. #3353's stale copies were just
  the foundation-PR commits before that PR was updated. The two unique
  #3353 commits (native runner + zmanian review fix) touch ONLY the
  wasm crate, so the inner crate's source has zero #3353 intent to
  preserve.
- `crates/ironclaw_wasm_product_adapters/{Cargo.toml,src/auth_verifier.rs,
  src/egress_policy.rs,src/lib.rs}` — took #3353's versions. These are
  the zmanian-review-hardened versions (proper error logging on clock
  failure, i128 overflow protection in HMAC replay window, trust-model
  warning docs, type-refactored egress policy).

## Adaptation required by the new product_adapters API

The post-sync product_adapters surface broke runner.rs in three ways:

1. `mark_*_verified` helpers moved behind `feature = "host-auth-mint"`
   → added `features = ["host-auth-mint"]` to the dep.
2. `ProductAdapter::parse_inbound` now takes `&ProtocolAuthEvidence`
   (borrowed) → added `&`.
3. `parse_inbound` now returns `ParsedProductInbound` (struct with
   payload variant) instead of `Result<Option<ProductInboundEnvelope>>`.
   The runner now builds `TrustedInboundContext::from_verified_evidence`
   + `ProductInboundEnvelope::from_trusted_parse` and pivots on
   `payload == ProductInboundPayload::NoOp` to decide between
   `Acknowledged` and `NoOp` outcomes. NoOp short-circuit preserves
   the original semantic (200 OK no-op for authenticated-but-ignored
   events) — the encoding just moved from `Option::None` to a payload
   variant.

Plus matching test-stub updates:
- `StaticAdapter`/`PanicAdapter` now store/return `ParsedProductInbound`
  and implement the new required `auth_requirement()` method + 4-arg
  `render_outbound` returning `ProductRenderOutcome::DeliveryRecorded`.
- `AckWorkflow`/`PendingWorkflow`/`PanicWorkflow`/`BlockingWorkflow`
  implement the new required `resolve_projection_subscription` method
  (returns a deterministic Internal error — runner tests never call it).
- `sample_envelope()` → `sample_parsed()` returning a non-NoOp
  `UserMessage` payload so workflow-path tests reach `accept_inbound`.

## Architecture boundary update

`ironclaw_architecture::reborn_dependency_boundaries::
wasm_product_adapter_crate_keeps_minimal_host_glue_dependencies`
fails-loud on unauthorized deps. The runner work adds 5 deps with
justified call sites:
- `async-trait`, `tokio`         — async ProductAdapter trait + Semaphore
- `chrono`                       — Utc::now() for TrustedInboundContext
- `hex`                          — HMAC signature encoding in verifier
- `tracing`                      — structured logging in hardened paths

Expected-set updated with an inline comment documenting each addition.
Also pruned `ironclaw_host_api`, `ironclaw_turns`, `serde`, `serde_json`,
`uuid` from the wasm crate's Cargo.toml — they were orphan deps with
zero call sites.

## Verified

- `cargo check --workspace` clean
- `cargo fmt --all -- --check` clean
- `cargo clippy --workspace --all-targets --all-features -- -D warnings`
  zero warnings
- `cargo test -p ironclaw_architecture` green (boundary rules satisfied)
- `cargo test -p ironclaw_product_adapters` green
- `cargo test -p ironclaw_wasm_product_adapters` 18 tests green
  (includes the 4 runner-behavior tests under the new API surface)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@henrypark133
henrypark133 requested a review from Copilot May 12, 2026 01:14
@nickpismenkov

Copy link
Copy Markdown
Contributor

Thanks for the quick re-review, @henrypark133. Pushed 35f29ce1f to address the WIT-manifest pair gap.

You're right that independent declared-egress-hosts / declared-credential-handles lists couldn't express the paired Rust contract — a future host glue would have had to either cross-product them (reintroducing the cross-pair leak) or invent pair metadata the manifest didn't carry. Good catch.

WIT changes

  1. New paired record:

    record declared-egress-target {
        host: string,
        credential-handle: option<string>,
    }

    Mirrors ironclaw_product_adapters::DeclaredEgressTarget on the Rust side. credential-handle: none declares an explicit unauthenticated target — the host's EgressPolicy::check now requires the exact (host, None) pair to authorize bare requests, so unauthenticated entries are deliberate, not catch-all.

  2. Manifest carries paired targets:

    record adapter-manifest {
        ...
        declared-egress-targets: list<declared-egress-target>,
        declared-auth-requirements: list<auth-requirement>,
    }

    Replaces the prior declared-egress-hosts: list<string> + declared-credential-handles: list<string>.

  3. Single index into the paired list:

    record egress-request {
        egress-target-index: u32,
        method: string,
        path: string,
        headers: egress-headers,
        body: list<u8>,
    }

    Replaces host-index: u32 + credential-handle-index: option<u32>. The cross-pair leak shape (slack_token against api.telegram.org) is now impossible to express over the WIT boundary — there's no separate handle index to mismatch the host with. Components select a single declared pair; the host applies both axes from that pair.

  4. Updated top-of-file host-invariant block to describe egress in pair terms — the WIT-side documentation of the Rust policy contract.

Boundary test

New wasm_product_adapter_wit_declares_egress_targets_as_paired_records in crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs:

  • Asserts record declared-egress-target and declared-egress-targets: list<declared-egress-target> exist.
  • Asserts the single egress-target-index: u32 shape.
  • Asserts the prior independent-list / split-index forms (declared-egress-hosts: list<string>, declared-credential-handles: list<string>, host-index: u32, credential-handle-index: option<u32>) are gone — a regression that splits the pair back fails this test loudly.

5 WIT boundary tests now pass (the new one plus the existing 4).

No Rust callers affected

Grep on crates/ and src/ confirms zero references to the removed WIT fields — wasmtime component bindgen is intentionally not wired up yet (per the WIT preamble). This is a contract update with no runtime call-site churn.

Verified

  • cargo check --workspace clean
  • cargo fmt --all -- --check clean
  • cargo clippy -p ironclaw_architecture -p ironclaw_wasm_product_adapters -p ironclaw_product_adapters --all-targets --all-features -- -D warnings zero warnings
  • cargo test -p ironclaw_architecture wasm_product_adapter 5 tests green

@serrrfirat

Copy link
Copy Markdown
Collaborator Author

Pushed 80e5e592 to tighten PR #3352 test coverage after a focused audit.

Added coverage for:

  • HMAC auth verifier missing signature and missing timestamp headers (ProtocolAuthFailure::Missing).
  • Egress origin-form path rejection, including explicit ://, fragment, backslash, CR/LF, and NUL shapes, plus a valid query-path control case.
  • Host-managed identity/proxy headers, including newly-forbidden Forwarded and X-Forwarded-For in addition to the existing host/auth/cookie/forwarded metadata headers.
  • Invalid header-name syntax (space, :, /, ., CRLF injection).

Verification:

  • cargo test -p ironclaw_wasm_product_adapters — 26 passed
  • cargo test -p ironclaw_product_adapters — lib 45 passed, integration suites 20 + 16 passed
  • cargo test -p ironclaw_architecture wasm_product_adapter — 5 passed
  • cargo fmt --all -- --check
  • cargo clippy -p ironclaw_wasm_product_adapters -p ironclaw_product_adapters -p ironclaw_architecture --all-targets --all-features -- -D warnings
  • git diff --check origin/reborn-integration...HEAD
  • cargo check --workspace
  • GitHub checks on 80e5e592 are green.

@henrypark133 henrypark133 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What looks good:

  • The prior WIT blocker is addressed on head 80e5e592: adapter-manifest now carries paired declared-egress-targets, and egress-request uses a single egress-target-index.
  • The old split declared-egress-hosts / declared-credential-handles and split indexes are now forbidden by wasm_product_adapter_wit_declares_egress_targets_as_paired_records.
  • The Rust egress policy still enforces exact pairs, rejects unauthenticated requests to credential-only hosts, and the egress header/path tests now cover more host-managed and injection shapes.
  • CI is green on the current head.

No verified findings.

Low-priority note:

  • Gemini's open SystemClock::now_unix_seconds comment about unwrap_or(0) is not blocker-grade for this PR. It is a narrow clock-abnormality behavior in verifier glue, not a credential/auth bypass, and the verifier fails closed for normal current timestamps if the system clock is nonsensical.

Summary:

  • Recommended verdict: Approve.
  • Prior feedback status: addressed.
  • Review coverage: current PR metadata/checks/reviews, open review threads, worktree-backed review of WIT, egress policy, egress DTOs, auth verifier, and architecture boundary tests.
  • Verification: git diff --check origin/reborn-integration...HEAD, cargo test -p ironclaw_wasm_product_adapters --quiet, cargo test -p ironclaw_product_adapters egress --quiet, cargo test -p ironclaw_product_adapters auth --quiet, cargo test -p ironclaw_architecture wasm_product_adapter -- --nocapture, cargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_hold -- --nocapture.

Conflict was in `Cargo.toml`'s workspace `members`:

- This PR (split/pr3316-02) adds `crates/ironclaw_wasm_product_adapters`.
- `reborn-integration` (since the previous sync) adds
  `crates/ironclaw_product_workflow`.

Resolved as the union — both crates are listed. Both directories
exist on disk after the auto-merge brought the new
`ironclaw_product_workflow` files in from `reborn-integration`.

Verified:
- `cargo check --workspace` clean
- `cargo fmt --all -- --check` clean
- `cargo clippy -p ironclaw_wasm_product_adapters -p ironclaw_product_adapters -p ironclaw_architecture --all-targets --all-features -- -D warnings` zero warnings
- `cargo test -p ironclaw_wasm_product_adapters -p ironclaw_product_adapters -p ironclaw_architecture` all suites green
  (45 product_adapters, 26 wasm_product_adapters, 13 architecture
  boundary tests including the WIT paired-target pin)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@nickpismenkov
nickpismenkov merged commit 5169dd5 into reborn-integration May 12, 2026
15 checks passed
@nickpismenkov
nickpismenkov deleted the split/pr3316-02-product-adapter-host-auth-egress branch May 12, 2026 18:20
nickpismenkov added a commit that referenced this pull request May 12, 2026
Conflicts in two files, both in `crates/ironclaw_wasm_product_adapters/src/`:

- `auth_verifier.rs` — `reborn-integration` has the post-#3352 state
  (the i128 timestamp arithmetic + the two regression tests for
  `i64::MIN`/`i64::MAX` extreme-timestamp overflow). This branch had
  the older zmanian-hardened version. Took `--theirs` — the
  post-#3352 state is a strict superset of #3353's older hardening.

- `egress_policy.rs` — `reborn-integration` has the post-#3352 state
  (pairwise `(host, Option<credential_handle>)` storage with
  `DeclaredEgressTarget`-shaped constructor, the
  `UnauthenticatedEgressNotDeclared` variant, and the cross-pair +
  unauthenticated-bypass regression tests). This branch had the older
  independent-set version. Took `--theirs` — the post-#3352 storage
  model strictly subsumes the older one and closes two leaks the
  older version had.

No external callers of `EgressPolicy::new` exist (verified via grep
on `crates/` and `src/`), so the signature change in the new file
has no ripple cost.

Verified:
- `cargo check --workspace` clean
- `cargo fmt --all -- --check` clean
- `cargo clippy -p ironclaw_wasm_product_adapters -p ironclaw_product_adapters -p ironclaw_architecture --all-targets --all-features -- -D warnings` zero warnings
- All test suites green: 45 product_adapters, 30 wasm_product_adapters
  (incl. the new extreme-timestamp regressions and cross-pair denial),
  13 architecture boundary tests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nickpismenkov added a commit that referenced this pull request May 12, 2026
Combines the `reborn-integration` merge with Henry's review fix —
the merge surfaced the same `ProductInboundEnvelope`/`ProtocolAuthEvidence`
API refactor that landed via my #3352 work, so the payload-normalization
code needed an update in the same commit.

## Merge

Conflicts:

- `Cargo.toml` workspace `members` — HEAD adds
  `ironclaw_telegram_v2_adapter`; `reborn-integration` adds
  `ironclaw_storage`, `ironclaw_loop_support`, `ironclaw_reborn`,
  `ironclaw_reborn_config`, `ironclaw_reborn_cli`,
  `ironclaw_product_workflow`, `ironclaw_outbound`, `ironclaw_llm`.
  Resolved as union — all directories exist on disk after auto-merge
  brought the new crates in.
- `Cargo.lock` — took `--theirs`; cargo settled the lockfile against
  the resolved Cargo.toml in `cargo check --workspace`.

## payload.rs API adaptation

`reborn-integration` carries the post-#3352 product-adapter API:

- `ProtocolAuthEvidence` is now a sealed struct (formerly an enum).
  Variant match `ProtocolAuthEvidence::{Verified,Failed} { .. }` →
  `is_verified()` / `failed(failure)` constructors / `claim()` accessor.
- `ProductInboundEnvelope` fields are now private. Direct struct
  literal → `ProductInboundEnvelope::from_trusted_parse(context, parsed)`
  where `context = TrustedInboundContext::from_verified_evidence(...)`
  and `parsed = ParsedProductInbound::new(...)`. Bulk-renamed
  `envelope.{external_event_id,external_actor_ref,external_conversation_ref,payload,...}`
  field accesses to method calls (envelope-side accessors).
- `mark_*_verified` helpers moved behind `feature = "host-auth-mint"`;
  added the feature to `ironclaw_telegram_v2_adapter`'s dev-deps so
  tests can mint verified evidence.

## Henry's High finding (review at 2026-05-12T00:58:29Z)

`classify_trigger` returned `ProductTriggerReason::DirectChat`
immediately for private chats, BEFORE checking `bot_command` entities.
A DM like `/help` reached `build_payload` with `trigger = DirectChat`,
which fell through the `BotCommand`-gated `Command` path and emitted
`ProductInboundPayload::UserMessage` — contradicting the adapter
advertising `InboundCommands` and silently downgrading every private
bot-command invocation.

Fix: recognize `bot_command` entities before the private-chat early
return. Non-command private messages still classify as `DirectChat`.

Two regression tests:

- `private_chat_recognized_bot_command_classifies_as_command` — Henry's
  exact `/help`-in-DM case asserts `Command{command="help", trigger=BotCommand}`.
- `private_chat_unknown_command_still_classifies_as_direct_chat` —
  defense-in-depth: an unrecognized `/nope` in a DM must still classify
  as `UserMessage` with `DirectChat`, not silently become a `Command`
  for a command the adapter doesn't recognize.

## Verified

- `cargo check --workspace` clean
- `cargo fmt --all -- --check` clean
- `cargo clippy -p ironclaw_telegram_v2_adapter --all-targets -- -D warnings` zero warnings
- `cargo test -p ironclaw_telegram_v2_adapter` 23 tests green
  (21 pre-existing + 2 private-chat command regressions)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nickpismenkov added a commit that referenced this pull request May 12, 2026
…I + copilot review

## Merge

Conflict in `crates/ironclaw_telegram_v2_adapter/Cargo.toml` — HEAD
had `tracing`, `uuid`, `ironclaw_wasm_product_adapters` in
`[dependencies]` and a dev-deps section with `test-support` feature;
`reborn-integration` had a leaner shape using `host-auth-mint`.

Resolved as a clean re-shape that ALSO addresses Copilot #4 (the
test-only deps belong in `[dev-dependencies]`): dropped `tracing` (no
src/ references), moved `uuid` to dev-deps (only used in
`#[cfg(test)] mod tests`), moved `ironclaw_wasm_product_adapters` to
dev-deps (only used by the integration contract test). Dev-deps now
enables BOTH `test-support` (for `FakeProductWorkflow` /
`FakeProtocolHttpEgress` / `FakeOutboundDeliverySink`) and
`host-auth-mint` (for `mark_*_verified` helpers).

## API port to post-#3352 `ironclaw_product_adapters`

The merge brought in the API refactor that landed via PR #3352. Ported
the v2 telegram adapter end-to-end:

- **`payload.rs`** — `parse_telegram_update` now returns
  `Result<ParsedProductInbound, _>` directly. NoOps encoded as
  `payload: ProductInboundPayload::NoOp` with synthetic external refs
  (`telegram_system` / `noop`) for cases where no real refs exist (no
  `message`, no `from`). This matches the new contract that says NoOps
  must be a parsed inbound with the explicit `NoOp` payload variant,
  not an out-of-band `None`. Dropped the `TelegramParsedInbound` enum
  and the `parse_telegram_update`-side envelope construction — that
  moves to the host runner per the new trust boundary
  (`TrustedInboundContext::from_verified_evidence` +
  `ProductInboundEnvelope::from_trusted_parse`). Removed `adapter_id`
  parameter (no longer needed) and `telegram_date_to_utc` (dead).

- **`adapter.rs`** — `ProductAdapter` trait conformance:
  - Added `auth_requirement()` method backed by a new
    `TelegramV2AdapterConfig.auth_requirement: AuthRequirement` field.
  - `parse_inbound` takes `&ProtocolAuthEvidence` and returns
    `Result<ParsedProductInbound, _>` directly.
  - `render_outbound` takes 4 args (added `&dyn OutboundDeliverySink`)
    and returns `Result<ProductRenderOutcome, _>` — `Deferred` for the
    no-op cases that previously returned `Ok(())`, `DeliveryRecorded`
    on success.
  - `ProductOutboundPayload::ProjectionSnapshot`/`ProjectionUpdate` are
    now struct variants — match arms updated.
  - `EgressResponse::status` is now an accessor method.
  - All `ProductAdapterError::*` variants with `reason` fields now
    expect `RedactedString::new(...)`.

- **`render.rs`** — `EgressRequest` is built via the new builder API
  (`EgressRequest::new(host, method, path).with_header(...).with_body(...).with_credential_handle(...)`).
  Extracted to a `build_egress_request()` helper so both
  `render_final_reply` and `render_progress_typing` share the construction.

## Copilot review findings addressed

- **Copilot #1 — `render.rs:45` (extra-segment validation):**
  `parse_reply_target` previously accepted reply targets like
  `tg:1:_:2:extra` and silently dropped the trailing segments. Added
  a final `segments.next().is_some()` check that rejects any reply
  target with more than the three documented segments
  (`chat_id:topic_id:reply_message_id`).

- **Copilot #4 — `Cargo.toml:29` (test-only deps):**
  Resolved during merge (see above). `tracing` dropped (no call sites
  in src/), `uuid` and `ironclaw_wasm_product_adapters` moved to
  dev-deps.

- **Copilot #2 — `tests/product_adapter_telegram_contract.rs:129`
  (alias-skip false-negatives) AND Copilot #3 — `:1053` (AC16
  doc/test mismatch):** scoped to the integration contract test file
  that depends on the full pre-#3352 API surface. Deferred — see
  below.

## Deferred: integration contract test surgery

`crates/ironclaw_telegram_v2_adapter/tests/product_adapter_telegram_contract.rs`
(~1700 lines, ~20+ test fixtures) was written against the pre-#3352
`ironclaw_product_adapters` API and needs case-by-case porting to the
new shape (`ProductInboundEnvelope` private fields,
`ProductOutboundEnvelope` with `target: ProductOutboundTarget`,
`projection_cursor: ProjectionCursor`, `EgressRequest` builder API,
`render_outbound` 4-arg signature, etc.).

Gated off with `#![cfg(any())]` at the top of the file with a
detailed comment explaining the scope. Once the test surgery lands,
removing the gate flips the file back on and Copilot #2 + #3 are
addressed in the same followup commit.

The library code, payload.rs unit tests, and adapter.rs unit tests
are all ported and green in this commit. **39 unit tests pass**
across the crate.

## Verified

- `cargo check --workspace` clean
- `cargo fmt --all -- --check` clean
- `cargo clippy -p ironclaw_telegram_v2_adapter --all-targets -- -D warnings` zero warnings
- `cargo test -p ironclaw_telegram_v2_adapter` 39 tests green (lib + payload + adapter inline + render)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nickpismenkov added a commit that referenced this pull request May 12, 2026
- Cargo.toml workspace: drop dead `channels-src-v2/telegram` exclude (Copilot #3)
- crates/ironclaw_telegram_v2_adapter/Cargo.toml: adopt 3355's post-cleanup
  shape — drop `tracing`, move `uuid` + `ironclaw_wasm_product_adapters`
  to dev-deps, dev-deps enable both `test-support` + `host-auth-mint`
- Sync adapter/payload/render/lib + integration test from 3355's branch
  so the stacked PR compiles against the post-#3352 product-adapter API
  (renders return `ProductRenderOutcome`, `EgressRequest` via builder, etc.)

Picks up the latest reborn-integration content; addresses Copilot #3
inline. Henry's persisted-active validator finding handled in the next
commit so this merge stays a pure reconciliation.
nickpismenkov added a commit that referenced this pull request May 12, 2026
Copilot review on PR #3355 (round 2): `TelegramV2Adapter` did not
override `ProductAdapter::declared_egress`, so it implicitly returned
the trait default `&[]`. Hosts that drive egress policy from
`DeclaredEgressTarget` (the WIT-paired `(host, credential_handle)`
shape introduced in #3352) would have denied every outbound Telegram
request despite `render_outbound` building requests for
`api.telegram.org`.

Fix:

- `TelegramV2Adapter` now stores a `Vec<DeclaredEgressTarget>` field
  populated in `new()` from the installation's
  `egress_credential_handle`. Single entry pairing
  `api.telegram.org` with `Some(<bot_token_handle>)` — matches the
  exact request shape that `render_outbound` builds via the
  `EgressRequest` builder in `render.rs`.
- New unit test
  `declared_egress_pairs_telegram_host_with_bot_token_handle` asserts
  the override surface.
- `telegram_declared_egress_hosts()` retained for tests and
  installation-agnostic host-list callers; doc comment now points
  production hosts at the trait method.

Also addresses Copilot's adjacent concern about the
`#![cfg(any())]`-gated contract suite. The reviewer asked for a real
feature flag (e.g. `cfg(feature = "contract-tests-todo")`), but the
workspace CI runs
`cargo clippy --all --tests --examples --all-features -- -D warnings`
— `--all-features` would enable any new feature and surface the 49
pre-existing port errors as lint regressions. Until the ~20+ fixtures
are migrated to the post-#3352 API, the file stays at `#![cfg(any())]`
with a fuller doc comment explaining why a feature flag is not
viable yet and what the port surface looks like. The substantive
fixture port (and Copilot #2 / #3 inside the file) remain followup
work.
nickpismenkov added a commit that referenced this pull request May 13, 2026
Henry's CHANGES_REQUESTED review on PR #3357: the "Test coverage"
section in `telegram-v2.md` claimed
`crates/ironclaw_telegram_v2_adapter/tests/product_adapter_telegram_contract.rs`
exists and covers all 16 acceptance bullets with `ac<N>_*` test
names. That file was removed on PR #3355 pending the post-#3352
fixture port, so the doc was making a false coverage claim.

Rewrite the section to describe the coverage that actually ships
today, organised by source surface:

- `payload::tests` (~24): private vs group routing, command
  classification including media captions and mention+command,
  unauthenticated payload, malformed JSON, missing `from`, topic-keyed
  conversation refs, photo attachments, control-char + oversized-arg
  rejection.
- `render::tests` (~4): reply-target round-trip, malformed target,
  `sendMessage` and `sendChatAction` shapes.
- `adapter::tests` (~15): capability default + progress opt-in,
  declared egress paired target, `parse_inbound` refusing unverified
  evidence, `render_outbound` install-scope guard, and the full
  `DeliveryStatus` mapping (Delivered / FailedRetryable /
  FailedUnauthorized / FailedPermanent / Deferred).
- `payload::slice_tests` (~8): UTF-16 entity offset slicing.

Explicitly call out the deferred integration contract suite, the
post-#3352 API surface the fixtures need migrated to, the retained
`tests/fixtures/*.json` payload set, and the expectation that the
restored tests carry `ac<N>_*` names referencing issue #3285 bullets.
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
Conflict was in `Cargo.toml`'s workspace `members`:

- nearai#3352 adds `crates/ironclaw_wasm_product_adapters`
- `reborn-integration` adds `crates/ironclaw_storage` and
  `crates/ironclaw_reborn_config`

Resolved as the union — all three crates are listed. The directories
for all three exist on disk after the auto-merge brought in the
sibling-side files. All other files merged cleanly.

Verified:
- `cargo check --workspace` clean
- `cargo fmt --all -- --check` clean
- `cargo clippy --workspace --all-targets -- -D warnings` zero warnings
- `cargo test -p ironclaw_architecture` (boundary tests) green
- `cargo test -p ironclaw_product_adapters` green

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
Merges split/pr3316-02-product-adapter-host-auth-egress (which itself
just synced with reborn-integration) into this branch. Most of the
diff is the upstream reborn-integration content nearai#3353 hadn't seen yet.

## Conflict resolution

22 paths conflicted. Resolution rules applied:

- `Cargo.toml` (workspace members) — took nearai#3352's post-sync version.
  No nearai#3353-unique workspace members.
- `Cargo.lock` — took nearai#3352's post-sync version; cargo settled it
  against the resolved Cargo.tomls.
- `crates/ironclaw_product_adapters/**` (15 files: CLAUDE.md, Cargo.toml,
  src/{adapter,auth,egress,error,external,fakes,identity,inbound,lib,
  outbound,projection,workflow}.rs, tests/product_adapter_contract.rs)
  AND `crates/ironclaw_wasm_product_adapters/wit/product_adapter.wit`
  — took nearai#3352's post-sync versions. nearai#3353's stale copies were just
  the foundation-PR commits before that PR was updated. The two unique
  nearai#3353 commits (native runner + zmanian review fix) touch ONLY the
  wasm crate, so the inner crate's source has zero nearai#3353 intent to
  preserve.
- `crates/ironclaw_wasm_product_adapters/{Cargo.toml,src/auth_verifier.rs,
  src/egress_policy.rs,src/lib.rs}` — took nearai#3353's versions. These are
  the zmanian-review-hardened versions (proper error logging on clock
  failure, i128 overflow protection in HMAC replay window, trust-model
  warning docs, type-refactored egress policy).

## Adaptation required by the new product_adapters API

The post-sync product_adapters surface broke runner.rs in three ways:

1. `mark_*_verified` helpers moved behind `feature = "host-auth-mint"`
   → added `features = ["host-auth-mint"]` to the dep.
2. `ProductAdapter::parse_inbound` now takes `&ProtocolAuthEvidence`
   (borrowed) → added `&`.
3. `parse_inbound` now returns `ParsedProductInbound` (struct with
   payload variant) instead of `Result<Option<ProductInboundEnvelope>>`.
   The runner now builds `TrustedInboundContext::from_verified_evidence`
   + `ProductInboundEnvelope::from_trusted_parse` and pivots on
   `payload == ProductInboundPayload::NoOp` to decide between
   `Acknowledged` and `NoOp` outcomes. NoOp short-circuit preserves
   the original semantic (200 OK no-op for authenticated-but-ignored
   events) — the encoding just moved from `Option::None` to a payload
   variant.

Plus matching test-stub updates:
- `StaticAdapter`/`PanicAdapter` now store/return `ParsedProductInbound`
  and implement the new required `auth_requirement()` method + 4-arg
  `render_outbound` returning `ProductRenderOutcome::DeliveryRecorded`.
- `AckWorkflow`/`PendingWorkflow`/`PanicWorkflow`/`BlockingWorkflow`
  implement the new required `resolve_projection_subscription` method
  (returns a deterministic Internal error — runner tests never call it).
- `sample_envelope()` → `sample_parsed()` returning a non-NoOp
  `UserMessage` payload so workflow-path tests reach `accept_inbound`.

## Architecture boundary update

`ironclaw_architecture::reborn_dependency_boundaries::
wasm_product_adapter_crate_keeps_minimal_host_glue_dependencies`
fails-loud on unauthorized deps. The runner work adds 5 deps with
justified call sites:
- `async-trait`, `tokio`         — async ProductAdapter trait + Semaphore
- `chrono`                       — Utc::now() for TrustedInboundContext
- `hex`                          — HMAC signature encoding in verifier
- `tracing`                      — structured logging in hardened paths

Expected-set updated with an inline comment documenting each addition.
Also pruned `ironclaw_host_api`, `ironclaw_turns`, `serde`, `serde_json`,
`uuid` from the wasm crate's Cargo.toml — they were orphan deps with
zero call sites.

## Verified

- `cargo check --workspace` clean
- `cargo fmt --all -- --check` clean
- `cargo clippy --workspace --all-targets --all-features -- -D warnings`
  zero warnings
- `cargo test -p ironclaw_architecture` green (boundary rules satisfied)
- `cargo test -p ironclaw_product_adapters` green
- `cargo test -p ironclaw_wasm_product_adapters` 18 tests green
  (includes the 4 runner-behavior tests under the new API surface)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
Addresses Copilot review comment on PR nearai#3352
(`crates/ironclaw_wasm_product_adapters/src/auth_verifier.rs:218`):
`SharedSecretHeaderAuth.verify` did not validate that the configured
`expected_secret` was non-empty. With an empty configured secret,
`ct_eq("", "")` returns true and any request with an empty header
value would authenticate.

Add the symmetric `is_empty()` guard already present on `HmacWebhookAuth`
(line 167): an empty configured secret short-circuits to
`ProtocolAuthFailure::Malformed` before any per-request comparison.
Placement at the top of `verify()` also makes the misconfiguration
rejection independent of what header value the attacker sends.

Regression test
`shared_secret_header_rejects_empty_expected_secret_as_malformed_config`
covers both the empty-header case (the documented exploit shape) and
the non-empty-header case (defense-in-depth — the rejection must not
depend on what arrives over the wire).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
Addresses henrypark133's two concerns on PR nearai#3352.

## EgressPolicy: pairwise (host, credential) authorization

`EgressPolicy` stored declared hosts and allowed credential handles as
independent `BTreeSet`s. When an adapter declared more than one
`(host, credential_handle)` pair, this authorized any allowed handle
against any declared host — e.g. an installation declaring
`(api.slack.com, slack_bot_token)` and
`(api.telegram.org, telegram_bot_token)` would also allow
`(api.telegram.org, slack_bot_token)`, leaking the Slack credential
to Telegram. This contradicts the `DeclaredEgressTarget` contract in
`ironclaw_product_adapters`, which is pair-shaped.

Internal storage is now `BTreeSet<(DeclaredEgressHost,
Option<EgressCredentialHandle>)>` keyed off the canonical
`DeclaredEgressTarget` shape. `check()` validates exact pair
membership; a handle declared with a different host now errors with
the new `CredentialHandleNotPairedWithHost { host, handle }` variant.
The legacy `UnauthorizedCredentialHandle` variant is preserved for
handles that aren't part of any declared pair.

`EgressPolicy::new` signature changed from two iterators (hosts +
handles) to a single `impl IntoIterator<Item = DeclaredEgressTarget>`.
No external callers — checked via workspace-wide grep.

Regression test `cross_pair_credential_handle_is_denied` seeds the
two-pair scenario from Henry's review and asserts both cross-pair
combinations (`slack_token → telegram.org` and
`telegram_token → slack.com`) return
`CredentialHandleNotPairedWithHost`. Added
`host_declared_without_credential_allows_handle_free_request` to pin
the `None`-credential pair semantics.

## HMAC verifier: i128 arithmetic for timestamp drift

`(now_secs - timestamp_secs).abs()` with `i64` arithmetic could panic
in overflow-checked builds when the untrusted timestamp header was
`i64::MIN` (`-9223372036854775808`): the subtraction overflows for
any nonnegative `now_secs`, and `i64::MIN.abs()` itself overflows.
The header value comes directly from a request, so this is reachable
— it would crash the verifier before it could return `Malformed`.

Parse the timestamp as `i128` and lift both `now_secs` and
`max_age_secs` through `i128::from(...)` for the drift computation.
`i128` has several orders of magnitude of headroom over any
plausible `i64` timestamp pair, so neither the subtraction nor the
abs() can overflow.

Two regression tests added (`hmac_verifier_rejects_extreme_negative_
timestamp_without_overflow` and the symmetric positive case) exercise
`i64::MIN` and `i64::MAX` header values and assert `Failed` is
returned without panic.

## Verified

- `cargo check --workspace` clean
- `cargo fmt --all -- --check` clean
- `cargo clippy -p ironclaw_wasm_product_adapters --all-targets --all-features -- -D warnings` zero warnings
- `cargo test -p ironclaw_wasm_product_adapters` 22 tests green

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
…3352

Henry's review at 2026-05-12T04:20:04Z flagged three issues against
the prior pairwise-egress + i128-timestamp commit (`8e3529f36`).

## High: unauthenticated egress bypass

`EgressPolicy::check` allowed `credential_handle: None` against any
declared host, even when the host was declared only with credentialed
pairs. An adapter declaring `[(api.telegram.org, Some(telegram_token))]`
and never declaring `(api.telegram.org, None)` was bypassed by an
unauthenticated request to that host — defeating the adapter's stated
"this host always requires a credential" contract.

Tightened `check()`: when `credential_handle == None`, require the
exact `(host, None)` pair to be in the declared targets. Added new
error variant `UnauthenticatedEgressNotDeclared { host }` distinct
from the credentialed-side errors. Two regression tests:
`unauthenticated_request_to_credential_only_host_is_denied` (the
bypass case Henry called out) and
`host_declared_with_both_pairs_admits_both_request_shapes` (pins the
admit-both contract so a future tightening doesn't force adapters to
pick one).

## Medium: hop-by-hop / transport-managed headers

`validate_header_name` in `crates/ironclaw_product_adapters/src/egress.rs`
forbade identity / proxy-metadata headers but permitted RFC 9110
§7.6.1 hop-by-hop headers (`connection`, `content-length`,
`transfer-encoding`, `te`, `trailer`, `upgrade`, `keep-alive`,
`proxy-connection`). An adapter could influence the host's HTTP
client lifecycle / message framing.

Expanded the FORBIDDEN list, grouped by purpose (identity-and-proxy
vs hop-by-hop-and-transport) with inline RFC reference. Two
regression tests: `egress_header_rejects_hop_by_hop_and_transport_
managed_headers` exercises each new entry plus mixed-case to pin the
case-insensitive lookup, and `egress_header_accepts_application_
headers` pins normal application headers so a future tightening
doesn't silently break adapter egress.

## Medium: WIT JSON-string shim quarantined

Per Henry's alternate suggestion ("explicitly quarantine JSON as a
temporary shim with boundary tests"). Added a top-of-file
"TEMPORARY: JSON-string payload shim" comment block in the WIT that
documents the shim status, the load-bearing host-side type check,
and points at the boundary test that pins the current shape. The new
boundary test `wasm_product_adapter_wit_pins_json_shim_shape` in
`reborn_dependency_boundaries.rs` asserts the five known shim fields
(`parsed-inbound.parsed-json`, `auth-evidence.evidence-json`,
`outbound-envelope.outbound-json`, `outbound-render.egress-request-
json`, `adapter-manifest.capabilities-json`) and the required
documentation tokens — so a typed redesign must update the test
deliberately rather than silently drifting away from the JSON shape.

Typed redesign (replacing the JSON strings with typed records/enums)
is intentionally deferred to a separate follow-up; this commit makes
the deferral explicit and reviewable.

## Verified

- `cargo check --workspace` clean
- `cargo fmt --all -- --check` clean
- `cargo clippy -p ironclaw_wasm_product_adapters -p ironclaw_product_adapters -p ironclaw_architecture --all-targets --all-features -- -D warnings` zero warnings
- `cargo test -p ironclaw_wasm_product_adapters -p ironclaw_product_adapters -p ironclaw_architecture` all suites green
  (egress_policy: 8 tests including 2 new; egress: 6 header tests
  including 2 new; architecture: 12 tests including 1 new WIT-shim
  pin)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
Henry's review (PR nearai#3352, 2026-05-12T05:04:30Z) flagged that the WIT
manifest still carried `declared-egress-hosts: list<string>` and
`declared-credential-handles: list<string>` as INDEPENDENT lists,
contradicting the Rust `EgressPolicy` that now requires exact
`(host, Option<credential_handle>)` pairs.

Independent lists couldn't express "Slack token only for Slack" —
the future host glue would have to either reconstruct pairs by
cross-product (reintroducing the cross-pair credential leak the
prior commit closed) or invent pair metadata the manifest didn't
carry.

## WIT changes

1. **New `declared-egress-target` record** with `host: string` and
   `credential-handle: option<string>`. Mirrors
   `ironclaw_product_adapters::DeclaredEgressTarget` on the Rust side.

2. **`adapter-manifest.declared-egress-targets: list<declared-egress-
   target>`** replaces the prior independent host + handle lists. An
   adapter that wants both authenticated and unauthenticated egress
   for the same host declares two targets (one with `credential-handle
   = some`, one with `credential-handle = none`) — matching the
   admit-both contract the Rust policy now pins.

3. **`egress-request.egress-target-index: u32`** replaces the prior
   `host-index: u32` + `credential-handle-index: option<u32>`. A
   component selects a single declared pair; the host applies both
   the host AND the (optional) credential from that single
   declaration. Cross-pair leak (e.g. `slack_token` against
   `api.telegram.org`) is now impossible to even *express* over the
   WIT boundary.

4. **Updated top-of-file host-invariant block** to describe egress
   in pair terms (the WIT-side documentation of the Rust policy
   contract).

No Rust callers exist for the prior fields — verified via grep on
`crates/` and `src/`. wasmtime component bindgen is intentionally
not wired up yet (per the WIT preamble), so this is a contract
update with no runtime call-site churn.

## Boundary test

New `wasm_product_adapter_wit_declares_egress_targets_as_paired_records`
in `crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs`:
- asserts `record declared-egress-target` and the paired
  `declared-egress-targets: list<declared-egress-target>` exist
- asserts the single `egress-target-index: u32` reference
- asserts the prior independent-list / split-index shape
  (`declared-egress-hosts: list<string>`, `declared-credential-
  handles: list<string>`, `host-index: u32`,
  `credential-handle-index: option<u32>`) is GONE — a regression
  that splits the pair back fails this test loudly.

## Verified

- `cargo check --workspace` clean
- `cargo fmt --all -- --check` clean
- `cargo clippy -p ironclaw_architecture -p ironclaw_wasm_product_adapters -p ironclaw_product_adapters --all-targets --all-features -- -D warnings` zero warnings
- `cargo test -p ironclaw_architecture wasm_product_adapter` 5 tests green
  (incl. the new pair-shape boundary test)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
…apter-host-auth-egress

feat(reborn): add product adapter host auth and egress primitives
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
Conflicts in two files, both in `crates/ironclaw_wasm_product_adapters/src/`:

- `auth_verifier.rs` — `reborn-integration` has the post-nearai#3352 state
  (the i128 timestamp arithmetic + the two regression tests for
  `i64::MIN`/`i64::MAX` extreme-timestamp overflow). This branch had
  the older zmanian-hardened version. Took `--theirs` — the
  post-nearai#3352 state is a strict superset of nearai#3353's older hardening.

- `egress_policy.rs` — `reborn-integration` has the post-nearai#3352 state
  (pairwise `(host, Option<credential_handle>)` storage with
  `DeclaredEgressTarget`-shaped constructor, the
  `UnauthenticatedEgressNotDeclared` variant, and the cross-pair +
  unauthenticated-bypass regression tests). This branch had the older
  independent-set version. Took `--theirs` — the post-nearai#3352 storage
  model strictly subsumes the older one and closes two leaks the
  older version had.

No external callers of `EgressPolicy::new` exist (verified via grep
on `crates/` and `src/`), so the signature change in the new file
has no ripple cost.

Verified:
- `cargo check --workspace` clean
- `cargo fmt --all -- --check` clean
- `cargo clippy -p ironclaw_wasm_product_adapters -p ironclaw_product_adapters -p ironclaw_architecture --all-targets --all-features -- -D warnings` zero warnings
- All test suites green: 45 product_adapters, 30 wasm_product_adapters
  (incl. the new extreme-timestamp regressions and cross-pair denial),
  13 architecture boundary tests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
Combines the `reborn-integration` merge with Henry's review fix —
the merge surfaced the same `ProductInboundEnvelope`/`ProtocolAuthEvidence`
API refactor that landed via my nearai#3352 work, so the payload-normalization
code needed an update in the same commit.

## Merge

Conflicts:

- `Cargo.toml` workspace `members` — HEAD adds
  `ironclaw_telegram_v2_adapter`; `reborn-integration` adds
  `ironclaw_storage`, `ironclaw_loop_support`, `ironclaw_reborn`,
  `ironclaw_reborn_config`, `ironclaw_reborn_cli`,
  `ironclaw_product_workflow`, `ironclaw_outbound`, `ironclaw_llm`.
  Resolved as union — all directories exist on disk after auto-merge
  brought the new crates in.
- `Cargo.lock` — took `--theirs`; cargo settled the lockfile against
  the resolved Cargo.toml in `cargo check --workspace`.

## payload.rs API adaptation

`reborn-integration` carries the post-nearai#3352 product-adapter API:

- `ProtocolAuthEvidence` is now a sealed struct (formerly an enum).
  Variant match `ProtocolAuthEvidence::{Verified,Failed} { .. }` →
  `is_verified()` / `failed(failure)` constructors / `claim()` accessor.
- `ProductInboundEnvelope` fields are now private. Direct struct
  literal → `ProductInboundEnvelope::from_trusted_parse(context, parsed)`
  where `context = TrustedInboundContext::from_verified_evidence(...)`
  and `parsed = ParsedProductInbound::new(...)`. Bulk-renamed
  `envelope.{external_event_id,external_actor_ref,external_conversation_ref,payload,...}`
  field accesses to method calls (envelope-side accessors).
- `mark_*_verified` helpers moved behind `feature = "host-auth-mint"`;
  added the feature to `ironclaw_telegram_v2_adapter`'s dev-deps so
  tests can mint verified evidence.

## Henry's High finding (review at 2026-05-12T00:58:29Z)

`classify_trigger` returned `ProductTriggerReason::DirectChat`
immediately for private chats, BEFORE checking `bot_command` entities.
A DM like `/help` reached `build_payload` with `trigger = DirectChat`,
which fell through the `BotCommand`-gated `Command` path and emitted
`ProductInboundPayload::UserMessage` — contradicting the adapter
advertising `InboundCommands` and silently downgrading every private
bot-command invocation.

Fix: recognize `bot_command` entities before the private-chat early
return. Non-command private messages still classify as `DirectChat`.

Two regression tests:

- `private_chat_recognized_bot_command_classifies_as_command` — Henry's
  exact `/help`-in-DM case asserts `Command{command="help", trigger=BotCommand}`.
- `private_chat_unknown_command_still_classifies_as_direct_chat` —
  defense-in-depth: an unrecognized `/nope` in a DM must still classify
  as `UserMessage` with `DirectChat`, not silently become a `Command`
  for a command the adapter doesn't recognize.

## Verified

- `cargo check --workspace` clean
- `cargo fmt --all -- --check` clean
- `cargo clippy -p ironclaw_telegram_v2_adapter --all-targets -- -D warnings` zero warnings
- `cargo test -p ironclaw_telegram_v2_adapter` 23 tests green
  (21 pre-existing + 2 private-chat command regressions)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
…I + copilot review

## Merge

Conflict in `crates/ironclaw_telegram_v2_adapter/Cargo.toml` — HEAD
had `tracing`, `uuid`, `ironclaw_wasm_product_adapters` in
`[dependencies]` and a dev-deps section with `test-support` feature;
`reborn-integration` had a leaner shape using `host-auth-mint`.

Resolved as a clean re-shape that ALSO addresses Copilot #4 (the
test-only deps belong in `[dev-dependencies]`): dropped `tracing` (no
src/ references), moved `uuid` to dev-deps (only used in
`#[cfg(test)] mod tests`), moved `ironclaw_wasm_product_adapters` to
dev-deps (only used by the integration contract test). Dev-deps now
enables BOTH `test-support` (for `FakeProductWorkflow` /
`FakeProtocolHttpEgress` / `FakeOutboundDeliverySink`) and
`host-auth-mint` (for `mark_*_verified` helpers).

## API port to post-nearai#3352 `ironclaw_product_adapters`

The merge brought in the API refactor that landed via PR nearai#3352. Ported
the v2 telegram adapter end-to-end:

- **`payload.rs`** — `parse_telegram_update` now returns
  `Result<ParsedProductInbound, _>` directly. NoOps encoded as
  `payload: ProductInboundPayload::NoOp` with synthetic external refs
  (`telegram_system` / `noop`) for cases where no real refs exist (no
  `message`, no `from`). This matches the new contract that says NoOps
  must be a parsed inbound with the explicit `NoOp` payload variant,
  not an out-of-band `None`. Dropped the `TelegramParsedInbound` enum
  and the `parse_telegram_update`-side envelope construction — that
  moves to the host runner per the new trust boundary
  (`TrustedInboundContext::from_verified_evidence` +
  `ProductInboundEnvelope::from_trusted_parse`). Removed `adapter_id`
  parameter (no longer needed) and `telegram_date_to_utc` (dead).

- **`adapter.rs`** — `ProductAdapter` trait conformance:
  - Added `auth_requirement()` method backed by a new
    `TelegramV2AdapterConfig.auth_requirement: AuthRequirement` field.
  - `parse_inbound` takes `&ProtocolAuthEvidence` and returns
    `Result<ParsedProductInbound, _>` directly.
  - `render_outbound` takes 4 args (added `&dyn OutboundDeliverySink`)
    and returns `Result<ProductRenderOutcome, _>` — `Deferred` for the
    no-op cases that previously returned `Ok(())`, `DeliveryRecorded`
    on success.
  - `ProductOutboundPayload::ProjectionSnapshot`/`ProjectionUpdate` are
    now struct variants — match arms updated.
  - `EgressResponse::status` is now an accessor method.
  - All `ProductAdapterError::*` variants with `reason` fields now
    expect `RedactedString::new(...)`.

- **`render.rs`** — `EgressRequest` is built via the new builder API
  (`EgressRequest::new(host, method, path).with_header(...).with_body(...).with_credential_handle(...)`).
  Extracted to a `build_egress_request()` helper so both
  `render_final_reply` and `render_progress_typing` share the construction.

## Copilot review findings addressed

- **Copilot #1 — `render.rs:45` (extra-segment validation):**
  `parse_reply_target` previously accepted reply targets like
  `tg:1:_:2:extra` and silently dropped the trailing segments. Added
  a final `segments.next().is_some()` check that rejects any reply
  target with more than the three documented segments
  (`chat_id:topic_id:reply_message_id`).

- **Copilot #4 — `Cargo.toml:29` (test-only deps):**
  Resolved during merge (see above). `tracing` dropped (no call sites
  in src/), `uuid` and `ironclaw_wasm_product_adapters` moved to
  dev-deps.

- **Copilot #2 — `tests/product_adapter_telegram_contract.rs:129`
  (alias-skip false-negatives) AND Copilot #3 — `:1053` (AC16
  doc/test mismatch):** scoped to the integration contract test file
  that depends on the full pre-nearai#3352 API surface. Deferred — see
  below.

## Deferred: integration contract test surgery

`crates/ironclaw_telegram_v2_adapter/tests/product_adapter_telegram_contract.rs`
(~1700 lines, ~20+ test fixtures) was written against the pre-nearai#3352
`ironclaw_product_adapters` API and needs case-by-case porting to the
new shape (`ProductInboundEnvelope` private fields,
`ProductOutboundEnvelope` with `target: ProductOutboundTarget`,
`projection_cursor: ProjectionCursor`, `EgressRequest` builder API,
`render_outbound` 4-arg signature, etc.).

Gated off with `#![cfg(any())]` at the top of the file with a
detailed comment explaining the scope. Once the test surgery lands,
removing the gate flips the file back on and Copilot #2 + #3 are
addressed in the same followup commit.

The library code, payload.rs unit tests, and adapter.rs unit tests
are all ported and green in this commit. **39 unit tests pass**
across the crate.

## Verified

- `cargo check --workspace` clean
- `cargo fmt --all -- --check` clean
- `cargo clippy -p ironclaw_telegram_v2_adapter --all-targets -- -D warnings` zero warnings
- `cargo test -p ironclaw_telegram_v2_adapter` 39 tests green (lib + payload + adapter inline + render)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
Copilot review on PR nearai#3355 (round 2): `TelegramV2Adapter` did not
override `ProductAdapter::declared_egress`, so it implicitly returned
the trait default `&[]`. Hosts that drive egress policy from
`DeclaredEgressTarget` (the WIT-paired `(host, credential_handle)`
shape introduced in nearai#3352) would have denied every outbound Telegram
request despite `render_outbound` building requests for
`api.telegram.org`.

Fix:

- `TelegramV2Adapter` now stores a `Vec<DeclaredEgressTarget>` field
  populated in `new()` from the installation's
  `egress_credential_handle`. Single entry pairing
  `api.telegram.org` with `Some(<bot_token_handle>)` — matches the
  exact request shape that `render_outbound` builds via the
  `EgressRequest` builder in `render.rs`.
- New unit test
  `declared_egress_pairs_telegram_host_with_bot_token_handle` asserts
  the override surface.
- `telegram_declared_egress_hosts()` retained for tests and
  installation-agnostic host-list callers; doc comment now points
  production hosts at the trait method.

Also addresses Copilot's adjacent concern about the
`#![cfg(any())]`-gated contract suite. The reviewer asked for a real
feature flag (e.g. `cfg(feature = "contract-tests-todo")`), but the
workspace CI runs
`cargo clippy --all --tests --examples --all-features -- -D warnings`
— `--all-features` would enable any new feature and surface the 49
pre-existing port errors as lint regressions. Until the ~20+ fixtures
are migrated to the post-nearai#3352 API, the file stays at `#![cfg(any())]`
with a fuller doc comment explaining why a feature flag is not
viable yet and what the port surface looks like. The substantive
fixture port (and Copilot #2 / #3 inside the file) remain followup
work.
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
Henry's CHANGES_REQUESTED review on PR nearai#3357: the "Test coverage"
section in `telegram-v2.md` claimed
`crates/ironclaw_telegram_v2_adapter/tests/product_adapter_telegram_contract.rs`
exists and covers all 16 acceptance bullets with `ac<N>_*` test
names. That file was removed on PR nearai#3355 pending the post-nearai#3352
fixture port, so the doc was making a false coverage claim.

Rewrite the section to describe the coverage that actually ships
today, organised by source surface:

- `payload::tests` (~24): private vs group routing, command
  classification including media captions and mention+command,
  unauthenticated payload, malformed JSON, missing `from`, topic-keyed
  conversation refs, photo attachments, control-char + oversized-arg
  rejection.
- `render::tests` (~4): reply-target round-trip, malformed target,
  `sendMessage` and `sendChatAction` shapes.
- `adapter::tests` (~15): capability default + progress opt-in,
  declared egress paired target, `parse_inbound` refusing unverified
  evidence, `render_outbound` install-scope guard, and the full
  `DeliveryStatus` mapping (Delivered / FailedRetryable /
  FailedUnauthorized / FailedPermanent / Deferred).
- `payload::slice_tests` (~8): UTF-16 entity offset slicing.

Explicitly call out the deferred integration contract suite, the
post-nearai#3352 API surface the fixtures need migrated to, the retained
`tests/fixtures/*.json` payload set, and the expectation that the
restored tests carry `ac<N>_*` names referencing issue nearai#3285 bullets.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: medium Business logic, config, or moderate-risk modules scope: dependencies Dependency updates scope: docs Documentation size: XL 500+ changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants