feat(reborn): Telegram v2 inbound tracer (webhook → ledger + binding, no reply)

[nickpismenkov]

Summary

Scope: Stands up the Reborn ProductAdapter / ProductWorkflow stack as an
inbound tracer for Telegram v2. Webhook delivery → shared-secret auth →
idempotency ledger → conversation binding → 200 ACK. The reply path is
intentionally stubbed; this PR does not produce outbound sendMessage.

Why no reply yet: the Reborn agent loop (DefaultInboundTurnService +
a real TurnCoordinator) ships across the still-open PRs #3544, #3550,
#3586. Without those, there is no concrete turn executor to invoke;
render_outbound has nothing to render. Wiring a canned/echo reply path
just to fire sendMessage would bake throwaway code into a production-marked
binary and create a non-trivial revert when the loop lands. We chose to ship
the inbound contract durably and merge the reply path in a focused follow-up
once the loop is in src/.

Operator visibility: the stub status is documented prominently in
crates/ironclaw_reborn_telegram_v2_host/CLAUDE.md
("Why 'stubbed reply path'") and in the module docs of
src/inbound_turn.rs.
On every inbound the host logs "Reborn host: inbound resolved + bound; reply path stubbed (no Reborn agent loop yet)" so it cannot be missed in
production.

Renamed from "Telegram v2 tracer — end-to-end webhook → reply" to better
match what the diff actually ships (per @serrrfirat's review:
#issuecomment-4454525610,
finding #1).

What this lands

Piece	File
libSQL V26 + Postgres V28 migrations (product_inbound_actions, product_bindings)	src/db/libsql_migrations.rs, migrations/V28__product_inbound_actions_and_bindings.sql
Durable IdempotencyLedger impls (both backends)	crates/ironclaw_product_workflow_storage/src/ledger_*.rs
Durable ConversationBindingService impls	crates/ironclaw_product_workflow_storage/src/binding_*.rs
TelegramHttpEgress shim (reqwest, declared-host allowlist, credential-handle resolve)	crates/ironclaw_product_workflow_storage/src/egress.rs
OutboundStateStoreDeliverySink (reuses existing ironclaw_outbound)	crates/ironclaw_product_workflow_storage/src/outbound_sink.rs
Standalone host crate (composition + axum router + serve loop), wired into the ironclaw-reborn binary behind a telegram-v2 Cargo feature	crates/ironclaw_reborn_telegram_v2_host/, crates/ironclaw_reborn_cli/src/commands/run.rs
Stub inbound turn service (resolves binding, returns synthetic Submitted, no reply)	crates/ironclaw_reborn_telegram_v2_host/src/inbound_turn.rs
Fail-closed default-namespace guard (rejects default / tenant_default / agent_default without explicit opt-in)	crates/ironclaw_reborn_telegram_v2_host/src/config.rs
Atomic create-or-bind in conversation binding service (no orphan threads on first-bind race)	crates/ironclaw_product_workflow_storage/src/binding_{libsql,postgres}.rs

What this does NOT land

• Outbound reply path. No call to render_outbound; no sendMessage;
  runtime.egress and runtime.outbound_store are constructed but unused.
• Reborn agent loop. Tracked in docs(reborn): agent loop skeleton framework spec + 9 workstream briefs #3544, arch(ws-0): state, checkpoints, BoundedRing, CapabilityCallSignature, NoProgressDetected #3550, arch(ws-6): canonical executor — strategy dispatch + checkpoint contract #3586.
• Echo / canned reply. Deliberately not implemented to avoid throwaway
  production code.
• WASM component-model runtime. Uses NativeProductAdapterRunner; swap
  to ProductAdapterComponentRuntime when Implement WASM ProductAdapter component runtime #3583 lands.

Design alignment

Follows the Reborn channel porting guide (docs/reborn/how-to-port-channel-to-reborn.md) Path C native-core shape and #3577 shared acceptance criteria for storage, host-mediated egress, credential handles, declared egress, redaction, default-off gates, and idempotency.

Two deliberate interim deviations, both anticipated by the design:

1. Native runner, not WASM component. NativeProductAdapterRunner (PR feat(reborn): add native product adapter runner #3353) until Implement WASM ProductAdapter component runtime #3583's WASM runtime ships. The porting guide explicitly permits native for parse/render in the tracer phase. Migration: swap one constructor call once Implement WASM ProductAdapter component runtime #3583 lands.
2. No reply path. Stub instead of DefaultInboundTurnService + real TurnCoordinator. See "Why no reply yet" above. Migration: drop StubInboundTurnService, wire the real services once docs(reborn): agent loop skeleton framework spec + 9 workstream briefs #3544 / arch(ws-0): state, checkpoints, BoundedRing, CapabilityCallSignature, NoProgressDetected #3550 / arch(ws-6): canonical executor — strategy dispatch + checkpoint contract #3586 land.

Test plan

• cargo fmt --all -- --check — clean
• cargo clippy --bin ironclaw --features libsql --tests -- -D warnings — clean
• cargo clippy -p ironclaw_product_workflow_storage --features libsql,postgres --tests -- -D warnings — clean
• cargo test -p ironclaw_product_workflow_storage --features libsql,postgres — passes
• cargo test -p ironclaw_reborn_telegram_v2_host — full e2e webhook → ledger settle + binding row + auth fail paths
• cargo test -p ironclaw_reborn_cli --all-features — CLI smoke (incl. regression that run enters the Telegram v2 path when TELEGRAM_BOT_TOKEN is set)
• cargo test -p ironclaw_architecture --test reborn_dependency_boundaries — dependency boundary tests pass

Review feedback addressed

@serrrfirat's #issuecomment-4454525610:

• Finding Move whatsapp channel source to channels-src/ for consistency #1 (High — reply not implemented): Resolved by narrowing PR scope and title. Stub is documented in code and CLAUDE.md; follow-up PR will wire the real DefaultInboundTurnService once the agent loop lands.
• Finding feat: adding Web UI #2 (Medium — binding/thread race): Fixed. Binding insert is now attempted before thread creation; on lost race, no orphan thread is created. Regression test added under contention.
• Finding Onboarding: show Telegram in channel selection and auto-install bundled channel #3 (Medium — namespace collision via defaults): Fixed. Startup now rejects literal default / tenant_default / agent_default unless IRONCLAW_REBORN_ALLOW_DEFAULT_NAMESPACE=1 is set (intended for dev/test only). Regression tests cover both the rejection path and the opt-in path.

Refs

• [Reborn] Migrate external channel adapters onto ProductAdapter contract #3285 ProductAdapter migration
• Track v1 (ironclaw) ports for legacy channels #3577 Reborn channel port tracker (Telegram = Path C)
• Define host-owned ingress boundary for Reborn HTTP surfaces #3578 host-owned ingress boundary
• Port WeChat channel to Reborn ProductAdapter #3582 WeChat port — this implementation is the exemplar
• [EPIC] Reborn Contributor Runway: enable parallel skill/tool/channel porting #3484 Reborn Contributor Runway (closes Child 10; partial Child 8)
• Implement WASM ProductAdapter component runtime #3583 WASM ProductAdapter component runtime (future migration)
• docs(reborn): agent loop skeleton framework spec + 9 workstream briefs #3544 / arch(ws-0): state, checkpoints, BoundedRing, CapabilityCallSignature, NoProgressDetected #3550 / arch(ws-6): canonical executor — strategy dispatch + checkpoint contract #3586 Reborn agent loop (reply path lands behind these)
• Porting guide: docs/reborn/how-to-port-channel-to-reborn.md

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces the 'Reborn' product-layer infrastructure, specifically enabling a Telegram v2 channel. It adds a new storage crate with libSQL and Postgres implementations for idempotency ledgers and conversation bindings, alongside a synthetic channel that bridges the new v2 workflow with the existing v1 agent runtime. Feedback focuses on improving the robustness and efficiency of the storage layer, including centralizing duplicated ID derivation logic, refining enum parsing, and optimizing Postgres operations through native UUID types and single-trip 'RETURNING' clauses. A minor correction was also suggested to use validated identifiers for internal runner lookups.

[gemini-code-assist]

The logic for deriving a UserId is duplicated between the libSQL and Postgres implementations. Additionally, there is a discrepancy between the documented format (colon-separated with a user: prefix at line 50) and the actual implementation (underscore-separated, no prefix). This logic should be moved to a shared location within the crate to ensure consistency and avoid duplication, especially for security-sensitive identifiers.

References

1. Use centralized helper functions for security-sensitive logic to ensure consistency and avoid duplication.

[gemini-code-assist]

The parse_phase function uses a fragile approach by wrapping the string in quotes and using serde_json::from_str. To improve robustness, implement parsing logic that handles case-insensitivity and trims whitespace. Additionally, prefer an exhaustive match statement over automated solutions to ensure that any new variants added to the enum are consciously handled at compile-time.

References

1. When parsing string inputs into enums, ensure the parsing logic is robust by handling case-insensitivity and trimming whitespace.
2. When mapping enum variants to another representation, prefer an exhaustive match statement over automated solutions to force a compile-time failure when new variants are added.

[gemini-code-assist]

The resolve_binding implementation performs an INSERT ... ON CONFLICT DO NOTHING followed by a separate SELECT to retrieve the thread_id. This can be optimized into a single database round-trip by using INSERT ... ON CONFLICT (adapter_id, installation_id, external_conversation_fingerprint, external_actor_kind, external_actor_id) DO UPDATE SET adapter_id = EXCLUDED.adapter_id RETURNING thread_id. This "fake update" pattern ensures the required data is always returned in one call, even when a conflict occurs.

[gemini-code-assist]

In Postgres, it is more efficient to use the native UUID type for the action_id primary key instead of TEXT. This reduces storage overhead and improves indexing performance for these identifiers.

[gemini-code-assist]

The runner map is currently keyed using the raw installation_id_str from the environment. It is safer to use the string representation of the validated installation_id (from line 93) to ensure that any normalization or validation performed by the AdapterInstallationId type is reflected in the lookup key.

Suggested change

	runners.insert(installation_id_str.to_string(), Arc::new(runner));
	runners.insert(installation_id.as_str().to_string(), Arc::new(runner));

[serrrfirat]

reborn related crates should not mingle with the current agent binary.

[nickpismenkov]

Addressed in 5667f19 — pulled the v2 boot orchestration out of src/main.rs into a single call:

ironclaw::channels::reborn::register_reborn_channels(
    ironclaw::channels::reborn::RebornChannelWiringInputs { … },
    &channels,
    &mut webhook_routes,
    &mut channel_names,
).await;

All Reborn-aware logic (secrets reads, storage construction, adapter assembly, route mounting) now lives in src/channels/reborn/registry.rs. src/main.rs no longer mentions specific Reborn types — bootstrap_telegram_v2, TELEGRAM_V2_CHANNEL_NAME, env-var parsing, all gone from the call site.

One honest caveat I'd want your read on: I interpreted "should not mingle" at the orchestration layer (no Reborn boot logic spread across main.rs). The Reborn crates are still listed in the root Cargo.toml because src/channels/reborn/*.rs uses them. If your intent is dependency-layer separation — i.e. the src/channels/reborn/ directory itself shouldn't exist inside the v1 agent binary's source tree, and should be its own workspace crate (something like crates/ironclaw_reborn_channels/ with main.rs depending on it via an optional Cargo feature) — that's a structural refactor I'm happy to do as a follow-up. Just let me know which read you want.

[nickpismenkov]

Pushed 5667f197a addressing all open review comments + landing the P1 round.

Code review comments — addressed

#	Reviewer	Where in 5667f197a
1	gemini-code-assist (derive_user_id duplicated)	New crates/ironclaw_product_workflow_storage/src/identifiers.rs; both binding_libsql.rs and binding_postgres.rs now call the shared helper
2	gemini-code-assist (parse_phase is fragile)	Exhaustive match in both ledger_libsql.rs and ledger_postgres.rs; paired comment on phase_to_str
3	gemini-code-assist (Postgres binding two-trip)	Single-roundtrip INSERT ... ON CONFLICT (...) DO UPDATE SET adapter_id = EXCLUDED.adapter_id RETURNING thread_id
4	gemini-code-assist (V28 action_id TEXT)	V28 uses UUID; Postgres ledger binds uuid::Uuid directly via tokio-postgres/with-uuid-1; libSQL keeps TEXT (no UUID type)
5	gemini-code-assist (runner map keyed by raw env)	runners.insert(installation_id.as_str().to_string(), ...) — uses the validated id
6	@serrrfirat (Reborn crates mingled with main binary)	New src/channels/reborn/registry.rs::register_reborn_channels; src/main.rs now has one function call + a RebornChannelWiringInputs struct, no direct Reborn type refs. Inline reply with an open question on orchestration- vs. dependency-layer interpretation.

P1 follow-ups (in-scope per pre-agreed triage)

• Postgres ledger + binding contract tests (4, gated on IRONCLAW_PRODUCT_STORAGE_POSTGRES_URL, skip-clean otherwise)
• V26 / V28 migration smoke tests (libSQL applied, _migrations recorded)
• Architecture boundary entry for ironclaw_product_workflow_storage
• Real bot identity from Telegram getMe at boot (fail-soft on errors)
• E2E accepted_inbound_settles_the_ledger_row proves ledger reaches phase='settled' after a webhook
• src/channels/reborn/CLAUDE.md + top-level mention

P2 — explicitly deferred to follow-up PRs

Per the triage we did: multi-installation support, in-binary setWebhook helper, and slash-command routing each get their own PRs (design choices distinct from the tracer scope).

Verification

cargo fmt --all -- --check                                                                  # clean
cargo clippy --bin ironclaw --features libsql --tests -- -D warnings                         # clean
cargo clippy -p ironclaw_product_workflow_storage --features libsql,postgres --tests -- -D warnings  # clean
cargo test -p ironclaw_product_workflow_storage --features libsql                            # 13/13
IRONCLAW_SKIP_POSTGRES_TESTS=1 cargo test --test postgres_contract ... --features libsql,postgres  # 4/4
cargo test --test reborn_telegram_v2_e2e --features libsql --no-default-features             # 7/7
cargo test --lib --no-default-features --features libsql migration_smoke                     # 2/2
cargo test -p ironclaw_architecture                                                          # 13/13

[serrrfirat]

why are we trying to wire the reborn v2 telegram channel into the current agent binary?

[nickpismenkov]

Just used it to manually verify that it actually works since reborn agent loop is not ready yet, will move to a separate workspace crate (crates/ironclaw_reborn_telegram_v2_host)

[zmanian]

Summary

Tracer-bullet wiring of the Reborn ProductAdapter/ProductWorkflow stack as a real second Telegram channel, gated by REBORN_TELEGRAM_V2_ENABLED. The boundary between the new storage crate (ironclaw_product_workflow_storage) and the in-binary wiring (src/channels/reborn/) is clean; the two interim bridge seams (V2InboundTurnService + synthetic ProductChannel) are well-isolated and explicitly documented. Live round-trip is verified and the test surface is strong for a tracer (router, idempotency replay, fail-closed auth, channel-manager round-trip, ledger settlement).

I have no blocking concerns. The points below are correctness/discipline items that should be addressed either in this PR or as named follow-ups before this becomes anything more than a default-off tracer.

Strengths

• Clean separation: storage crate has no v1 dependencies; src/channels/reborn/registry.rs is the single entry point from main.rs, addressing serrrfirat's "Reborn crates mingled with main binary" concern.
• Credential discipline: bot token never reaches the adapter; EgressCredentialHandle is resolved inside TelegramHttpEgress at transit time. The declared-host allowlist gates before any network call. unresolved_credential_is_rejected test locks this in.
• Identifier discipline: typed newtypes throughout (AdapterInstallationId, ProductAdapterId, EgressCredentialHandle, TenantId, AgentId, UserId, ThreadId). The derive_user_id helper is centralized in identifiers.rs per gemini's prior comment, with an explicit security-relevance docstring.
• Fail-closed webhook auth: invalid_secret_returns_401 test, AuthenticationFailed → 401, retryable vs non-retryable separation in v2_router::handle.
• Idempotency contract is exercised end-to-end: duplicate_update_replays_through_ledger and accepted_inbound_settles_the_ledger_row cover the New/Replay/Settled state transitions.
• Boot is robust: missing secrets warn and skip wiring (does not crash boot); getMe failure falls back to placeholder bot identity with a warn (does not crash boot); explicit comments document the consequence (group-chat triggers may misclassify until restart).
• Dual-backend parity: V26 (libSQL) and V28 (Postgres) migrations are aligned; unique constraints match; parse_phase / phase_to_str exhaustive on ActionPhase in both backends.

Issues

Critical

None.

Major

1. Ledger begin_or_replay is TOCTOU across both backends. Both LibSqlProductIdempotencyLedger::begin_or_replay (ledger_libsql.rs:167) and PostgresProductIdempotencyLedger::begin_or_replay (ledger_postgres.rs:94) do SELECT … WHERE fingerprint followed by an unconditional INSERT. Two concurrent webhooks with the same (adapter, installation, source_binding_key, external_event_id) will both miss the SELECT, both attempt INSERT, and one will fail on the UNIQUE constraint with a raw DB error rather than the expected Transient { "already in flight" } reply. Telegram retries reuse update_id, so this race is reachable in production under retry pressure. The binding layer's Postgres impl was already moved to single-roundtrip upsert in this PR (per the changelog) and the libSQL binding handles SqliteFailure(19, _) explicitly — apply the same discipline to the ledger:

   • libSQL: catch ::libsql::Error::SqliteFailure(19, _) from the INSERT and re-fetch the row, returning Replay or Transient based on the now-visible phase. (Mirrors binding_libsql.rs:173.)
   • Postgres: switch to INSERT … ON CONFLICT (adapter_id, installation_id, source_binding_key, external_event_id) DO NOTHING RETURNING action_id, and if no row is returned, SELECT the existing row and decide Replay vs Transient. (Mirrors the binding's new pattern.)

   This is the same anti-pattern flagged across the codebase in .claude/rules/review-discipline.md ("Transaction safety" / "Fix the pattern, not the instance"). The unit test second_begin_while_in_flight_is_transient passes only because it's serial; add a concurrent test driving two begin_or_replay calls in parallel.

2. Outbound tokio::spawn in ProductChannel::respond is unbounded and untracked. product_channel.rs:150 spawns the render+POST in a detached task and returns Ok(()) immediately. There is no bound on in-flight tasks, no JoinHandle retained, no per-installation semaphore, and no observability that one has hung. A misbehaving Telegram API (timeouts at 30s × N concurrent agent replies) will accumulate tasks indefinitely. Recommend an Arc<Semaphore> with a configurable in-flight cap (default ~16) per ProductChannel, and either a tracked JoinSet or at minimum a counter exposed via the existing observability hooks. References: .claude/rules/safety-and-sandbox.md "Bounded Resources" — "Tokio task fan-out … bounded semaphore on spawns driven by user input."

3. bot_token is held in memory as a plain String for the lifetime of the runner. boot.rs:67 calls .expose().to_string() and the clone flows into StaticCredentialResolver (which stores it as String). The host secrets store presumably has zeroize semantics; once exposed into a plain String, that's lost. The dedicated EgressCredentialResolver trait is the right seam to fix this — replace StaticCredentialResolver in the bootstrap path with one that re-reads the secret via the SecretsStore on every resolve() (or caches a secrecy::SecretString). At minimum, document the lifecycle explicitly. For a tracer this is acceptable; for production it shouldn't ship.

4. V28 migration numbering jump from V9. migrations/V28__...sql jumps over V10–V27. Per .claude/rules/database.md, "always number after the highest version on staging/main." git ls-tree origin/staging migrations/ shows V9 as the highest. The jump may be intentional (reserving numbers for in-flight branches) but should be justified in the PR body — and if any V10–V27 lands on staging before this merges, you have a conflict. Confirm with the migration owner that V28 is allocated for this PR, and if not, renumber. The libSQL counterpart uses V26 which has the same risk in INCREMENTAL_MIGRATIONS.

Minor

5. Phase parse/format pair is duplicated between ledger_libsql.rs and ledger_postgres.rs. Both files redefine parse_phase and phase_to_str with identical bodies. The shared identifiers.rs module is a natural home for these too — or a dedicated phase.rs. Same drift risk gemini flagged on derive_user_id. Lock-step comments help but don't enforce.

6. ProductChannel::respond swallows errors via tracing::warn! only. product_channel.rs:168 logs and discards. There's no OutboundDeliveryAttempt recorded for synthesis/build errors (e.g. rebuild conversation_ref failure, thread id parse failure). A failure between the agent producing a reply and adapter.render_outbound being called leaves no audit row, so reconciliation jobs can't see it. Consider writing a synthetic DeliveryStatus::FailedPermanent to the sink before returning, so the audit trail is complete.

7. enable_non_cli is checked at the top of register_reborn_channels (registry.rs:39) so the CLI runs do not wire Reborn. No issue, but flag: there is no doctor/status output today that reports "v2 enabled but skipped because non_cli=false." Consider adding a tracing::debug! so an operator running ironclaw --cli-only with the v2 flag set isn't silently surprised.

8. OutboundStateStoreDeliverySink::record swallows store errors via warn. outbound_sink.rs:103 is documented ("Persist failure must not unwind a successful HTTP send") and the comment is reasonable — but no metric is emitted, only a log line. Reconciliation cannot drive off log lines. Track in a follow-up: emit a DeliverySink::record_failed counter or surface via Observer.

9. fetch_bot_identity parses result.id as i64. The Telegram bot user id is well below 2^63, but the response field is per-spec a 64-bit signed integer — fine. The bot_user_id = 0 fallback is reasonable; just be aware that 0 is also a valid value range (it isn't actually used by Telegram, but worth a comment that 0 is reserved as the sentinel).

10. product_channel.rs:122 silently drops malformed v2_topic_id / v2_reply_target_message_id. .and_then(|s| s.parse::<i64>().ok()) collapses a parse error to None. This is .ok()? pattern banned by .claude/rules/error-handling.md for DB/IO but is acceptable here since these are optional fields recovered from metadata — please add a // silent-ok: optional metadata, falls back to top-level chat reply comment per the convention.

11. #[doc(hidden)] pub fn with_base_url_for_test on TelegramHttpEgress (egress.rs:69) is the right pattern, but consider gating behind #[cfg(any(test, feature = "test-support"))] — doc(hidden) is not a compile-time barrier.

Suggestions

• Add a concurrent-begin regression test for the ledger once the TOCTOU fix lands. Pattern: spawn 4 tasks calling begin_or_replay on the same fingerprint; assert exactly one returns New and the rest return Replay or Transient. This is exactly the case .claude/rules/testing.md "Test through the caller" calls out — the unit tests pass while the runtime races.
• Surface a metrics_descriptor for product_inbound_actions phase counts (and outbound_delivery_attempts status counts) so the existing observability subsystem can scrape them without bespoke wiring.
• The CLAUDE.md for src/channels/reborn/ is excellent — recommend adding a one-line note about the TOCTOU/concurrent-retry semantics next to the "Known gaps" list once the ledger fix lands.
• The "two deliberate deviations" framing in the PR body and CLAUDE.md is exactly right — keep the migration receipts in the body for Implement WASM ProductAdapter component runtime #3583 / Reborn loop landings.

Verdict

Approve with major-tier follow-ups. This is a strong tracer that lands the durable ledger/binding/egress contract, plumbs it end-to-end against both DB backends, and verifies live round-trip. The bridge seams are honest about their interim status. Major items (1) ledger TOCTOU, (2) unbounded spawn, (3) plaintext bot token cache should each get a dedicated follow-up issue tracked against this PR before any non-default-off rollout. Item (4) needs confirmation in this PR. Default-off gating and the existing v1/v2 exclusivity guard make merging now safe.

[henrypark133]

Review: fix durable retry recovery before merging

The standalone-host extraction is the right direction: the v1 agent binary no longer owns this Reborn Telegram tracer, the storage/host split is clearer, and the concurrent-insert TOCTOU fix is a good improvement. I still think this needs changes before merge because the durable idempotency contract can permanently wedge Telegram retries after a timeout or crash.

What looks good:

• The latest commit cleanly removes the Reborn Telegram host from the v1 agent binary and keeps the tracer in its own workspace crate.
• The ledger insert path now handles concurrent duplicate deliveries via insert-first / ON CONFLICT patterns instead of surfacing raw unique-constraint errors.
• Local targeted checks passed for me: cargo test -p ironclaw_product_workflow_storage --features libsql --lib, cargo test -p ironclaw_reborn_telegram_v2_host --features libsql, and cargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_hold.

Critical: non-terminal idempotency rows can retry-block forever

IdempotencyLedger explicitly requires durable implementations to reclaim non-terminal Received / Dispatched reservations through a recovery lease, TTL, or equivalent sweeper so a process crash or transient dispatch failure cannot leave the fingerprint permanently blocked. The new libSQL and Postgres ledgers record received_at, but begin_or_replay never checks age and there is no sweeper or lease handoff. Once a row is stuck in received or dispatched, both backends just return Transient("idempotency fingerprint already in flight; retry after recovery lease") forever.

That is reachable in this host: NativeProductAdapterRunner::process_webhook reserves the fingerprint inside workflow handling, and on workflow timeout it aborts the spawned task. A timeout, panic boundary, process crash, or cancellation after reservation but before settle/release leaves the row non-terminal. Telegram will retry the same update_id, but every retry hits the same row and receives a retryable error forever, so the message is never accepted and never replayed.

Expected fix direction: implement the recovery contract in both durable backends. For example, add a configured in-flight lease TTL and have begin_or_replay atomically reclaim stale received/dispatched rows before returning New, or add a sweeper with caller-level tests that prove a stale non-terminal ledger row is eventually reprocessable. Please cover both libSQL and Postgres, and include a test that drives the actual begin_or_replay path with an expired non-terminal row rather than only testing the helper shape.

Concerning: getMe failures can leak the bot token through logged URLs

fetch_bot_identity builds https://api.telegram.org/bot{token}/getMe and then formats reqwest errors into strings that are logged by the caller. Reqwest errors commonly include the request URL, so a DNS/TLS/connect failure can put the bot token into logs despite the surrounding SecretString handling. This path should sanitize before logging, e.g. use err.without_url() / err.to_string() after URL removal, or avoid carrying token-bearing URLs in formatted error strings at all.

Concerning: missing LIBSQL_PATH silently uses in-memory durable storage

When Postgres is not configured, the host defaults libSQL to :memory: if LIBSQL_PATH is absent. For a binary whose purpose is durable idempotency and conversation binding, that is a dangerous production footgun: a restart drops the ledger and bindings, so duplicate webhook delivery and conversation continuity semantics silently change. Please require an explicit persistent storage path by default, or gate :memory: behind an unmistakable dev/test-only env var.

Low-priority notes:

• The Reborn docs are stale after the standalone-host extraction. docs/reborn/contracts/telegram-v2.md and docs/reborn/contracts/product-adapters.md still describe REBORN_TELEGRAM_V2_ENABLED, v1/v2 exclusivity through ironclaw::config::validate_telegram_v1_v2_exclusivity, fake services below the workflow facade, and production wiring as not implemented. Those no longer match this PR and should be updated or clearly scoped as historical status.

[nickpismenkov]

@henrypark133 could you please take a look again

[zmanian]

Re-review (post 2b20637)

Following up on my prior re-review (#3590 (comment), verdict APPROVE with the plaintext-token residual tracked as #3606). Three new commits land on top of face166: af0ef69, 40a160f, 1f433f7, plus the merge 2b20637. None regress prior approvals; each closes a real defect that is in scope for this PR.

40a160f — atomic create-or-bind

The fix is correct on both backends and well-matched: reserve the binding row with a pre-minted ThreadId first, only call ensure_thread once you have won the INSERT. libSQL catches SqliteFailure(2067) (the extended SQLITE_CONSTRAINT_UNIQUE code — note this is the same extended-code bug pattern the earlier PR-3590 commit caught in the ledger; reusing the right code here is what makes this work). Postgres uses INSERT ... ON CONFLICT DO NOTHING RETURNING thread_id with query_opt as the win/loss signal. A rollback_binding cleans up the binding row if ensure_thread fails after the INSERT win, so we never leave a binding pointing at a missing thread. The lost-race lookup_existing re-SELECT returns canonical state; an ok_or_else makes the "INSERT lost but no canonical row" case fail loud rather than silently mis-bind.

The regression test (concurrent_first_bind_creates_exactly_one_thread) is exactly the right shape: 16 concurrent resolve_binding calls, asserts both that every caller sees the same canonical thread_id and that ensure_thread is invoked exactly once (via a counting decorator on SessionThreadService). This is the caller-level coverage the .claude/rules/testing.md "Test Through the Caller" rule asks for. Postgres equivalent reasonably deferred to integration tier.

One thin spot: the libSQL re-SELECT runs on the same Connection as the INSERT. In libSQL/SQLite, in autocommit mode without an explicit transaction, the winner's INSERT is durable before our re-SELECT sees the conflict, so the loser will read it. Worth a single-line comment confirming the assumption (no transaction wrapping these statements), since the symmetric Postgres path also runs on a single pooled client and is autocommit; not a blocker.

1f433f7 — fail closed on default namespace IDs

This is the right fix and a real escalation vector. Prior posture: installation_id, tenant_id, agent_id defaulted to literal strings (default / tenant_default / agent_default). Since installation_id is part of every binding-uniqueness key and feeds derive_user_id, two hosts started against the same DB with all three unset would collapse different bots into the same canonical UserId — distinct Telegram chats observing one user namespace, with all the cross-tenant data-bleed that implies. The new guard requires all three to be explicitly set, surfaces the missing names in HostError::Config, and supports an explicit IRONCLAW_REBORN_ALLOW_DEFAULT_NAMESPACE=1 opt-in that mirrors IRONCLAW_REBORN_ALLOW_EPHEMERAL. The env_flag_set helper correctly treats =0 as unset.

Refactoring from_env into from_env_with(EnvLookup) to inject a fake env map (rather than mutating process globals) is the right shape — std::env::set_var is unsafe under multi-threaded test scheduling, and your tests now exercise the exact branches without that risk. 5 unit tests cover the matrix cleanly (explicit accepted, single missing rejected, multiple missing rejected, opt-in accepted, opt-in=0 not bypassing).

af0ef69 — consolidate Telegram v2 host into ironclaw-reborn

Substrate independence is preserved. I verified:

• Root Cargo.toml's [dependencies] block contains zero ironclaw_reborn*, ironclaw_product_*, or ironclaw_telegram_v2_* entries. The v1 agent binary still has no compile-time path to any Reborn crate.
• The Reborn coupling lives entirely in ironclaw_reborn_cli, gated behind a telegram-v2 Cargo feature with proper optional = true + default-features = false and feature-forwarding via the ? syntax (ironclaw_reborn_telegram_v2_host?/libsql) so libSQL/Postgres aren't pulled in when telegram-v2 is off.
• The architecture-boundary test (reborn_cli_binary_crate_stays_separate_from_v1_root) was updated to allow ironclaw_reborn_telegram_v2_host as an explicit, justified exception, with the inline rationale spelling out that future per-channel hosts (Slack/Discord/WeChat) join the same list — that is the right way to evolve the allow-list rather than to weaken it.
• --no-default-features (or --no-default-features --features libsql) builds an ironclaw-reborn without any Telegram crate at all; the smoke test run_initializes_minimal_runtime_shell_without_touching_v1_state now explicitly env_remove("TELEGRAM_BOT_TOKEN") to keep the runtime-shell snapshot path under test.
• The new caller-level test run_enters_telegram_v2_path_when_bot_token_is_set proves the wire-up: setting the token and withholding the webhook secret forces a HostConfig::from_env failure, which the test pattern-matches on stderr while asserting the runtime-shell banner did not print. This is exactly the regression coverage the "test through the caller" rule asks for.

One minor: telegram_v2_configured_in_env() keys solely on TELEGRAM_BOT_TOKEN. If an operator has that var set in their shell for unrelated reasons (e.g. CI seeding it for a different binary), ironclaw-reborn run silently changes behavior. The fail-closed namespace guard in 1f433f7 mitigates this — they get a loud startup error rather than a quiet boot — and the smoke test documents the contract. Worth a one-line note in the CLI help for run ("when TELEGRAM_BOT_TOKEN is set, run enters the Telegram v2 serve loop"), but non-blocking.

Open items, unchanged

• Eliminate residual plaintext bot token in StaticCredentialResolver / SharedSecretHeaderAuth #3606 (plaintext bot token) — same status as my prior re-review: SecretString wraps the token at the HostConfig boundary, but it's still .expose_secret().to_string() into StaticCredentialResolver's plain String. The inline comment on HostConfig::telegram_bot_token accurately reflects this and references item Onboarding: show Telegram in channel selection and auto-install bundled channel #3. Pre-rollout follow-up, not a blocker for this PR.
• Host-owned schema duplication of the storage crate's assumed shape, with no lock-step test — also unchanged from my prior re-review. Still worth a follow-up issue.

Verdict

APPROVE. The three new commits are clean, narrowly scoped, well-tested, and each closes a real defect I or @serrrfirat flagged. Substrate independence is preserved (the v1 binary still has zero Reborn crate awareness). #3606 remains the right follow-up for the plaintext token residual.

[serrrfirat]

Architecture alignment request: this PR is the main place where channel-specific drift can appear, so please route Telegram v2 through the shared ProductWorkflow surface instead of growing a Telegram-specific ledger/binding/turn pipeline.

Desired shape:

• Telegram host/adapter owns protocol parsing, auth verification, and outbound rendering/egress.
• After inbound parse/auth, Telegram should call ProductWorkflow::accept_inbound.
• ProductWorkflow should own product inbound idempotency, binding resolution, terminal rejection replay, and turn submission handoff.
• Durable storage from this PR should back the shared ProductWorkflow ports where needed, rather than becoming a separate Telegram-only implementation path.

Follow-up PR #3727 prepares that shared ProductWorkflow route for the initial rollout slice: trusted installation-to-tenant/default-scope mapping, ironclaw_conversations binding, projection subscription binding validation, and in-flight idempotency semantics. Please align this PR with #3727 so Telegram and future channels do not each build their own facade at the composition root.

[zmanian]

Re-review of 4 new commits since prior round

Range covered: db68eae8..2b206374 (4 substantive commits + merge, landed 2026-05-14 → 2026-05-15). I have not reviewed this PR before under this account, so prior-finding tracking below is against @henrypark133's CHANGES_REQUESTED review (2026-05-14 01:09Z) and @serrrfirat's inline comments, since the author's comments reference an anticipated zmanian review.

Status of prior findings

1. Recovery lease for non-terminal idempotency rows — Resolved. ledger_libsql.rs and ledger_postgres.rs now carry recovery_lease: Duration (default constant in recovery.rs), and begin_or_replay reclaims stale received / dispatched rows via a guarded UPDATE on (action_id, received_at) so two racing reclaimers can't both win. Tests drive the actual begin_or_replay path with both fresh and expired rows, on both backends. Contract is now reachable on Telegram retries after a process crash.

2. getMe URL leaking the bot token in reqwest error strings — Resolved. fetch_bot_identity in boot.rs routes every reqwest::Error through a scrub helper that calls .without_url() before to_string(). JSON/HTTP-status branches stringify only response.status() or body, neither of which carries the token.

3. Silent :memory: libSQL fallback — Resolved. config.rs resolve_storage now requires either DATABASE_URL or LIBSQL_PATH; absence of both returns a Config error unless ALLOW_EPHEMERAL_LIBSQL=1 is set, in which case a warn! documents the choice. Acceptable.

4. Default-namespace collision (installation_id / tenant_id / agent_id) — Resolved (1f433f73). Fail-closed unless every var is set or ALLOW_DEFAULT_NAMESPACE=1 opt-in. Good.

5. Orphan thread when concurrent inbound loses the binding-insert race — Resolved (40a160f8). Both backends pre-mint the candidate ThreadId, INSERT the binding first, and only call ensure_thread after winning. Loser path goes through lookup_existing without ever creating a thread. Winner failure path calls rollback_binding. Verified for libSQL SqliteFailure(2067, _) (extended UNIQUE code, not bare 19) and the Postgres ON CONFLICT … DO NOTHING RETURNING pattern. Concurrency test (binding_libsql.rs:548) exists.

6. v1/v2 wiring inside agent binary (@serrrfirat) — Resolved. af0ef699 consolidates the host into crates/ironclaw_reborn_telegram_v2_host and removes it from src/main.rs. The dependency-boundary test (reborn_dependency_boundaries.rs) enforces it.

7. Stale docs after extraction — Partial. docs/reborn/contracts/telegram-v2.md was updated, but I'd want a spot-check that product-adapters.md no longer says production wiring is unimplemented. Not blocking.

Re-verifying the tracer-specific invariants

• Atomic create-or-bind: confirmed above (finding #5).
• No-reply mode: confirmed. boot.rs constructs the adapter with progress_push_enabled: false and never wires an OutboundDeliverySink. NativeProductAdapterRunner::process_webhook returns Acknowledged { ack } only; the workflow path for ProductInboundPayload::NoOp and user-message acks does not invoke any sink. There is no sendMessage call anywhere in crates/ironclaw_reborn_telegram_v2_host/**. The tracer cannot reply by construction.
• Webhook signature validation: SharedSecretHeaderAuth checks X-Telegram-Bot-Api-Secret-Token before parse_inbound. Auth-strategy mismatch is rejected pre-parse. Good. Note that expected_secret is held as a plain String in the runner for process lifetime — author has annotated this as residual exposure; acceptable for tracer rollout, must be revisited before non-default-off.
• Idempotency on duplicate Telegram updates: the same update_id produces the same fingerprint, which now goes through the durable ledger with recovery-lease semantics. Retries after crash are now replayable.

New findings (non-blocking)

• boot.rs falls back to placeholder ("ironclaw_telegram_v2_unknown", 0) when getMe fails. The bot_user_id = 0 placeholder is used in GroupTriggerPolicy and will silently misclassify group-chat triggers until restart. Operationally surprising on transient startup network errors. Consider failing closed and exiting, or at least surfacing a metric/health-check bit so an operator notices.
• expected_secret and telegram_bot_token both end up cloned as plain String inside long-lived runner / resolver state. Author has called this out in comments. Track as a follow-up before non-tracer rollout; not a blocker for a no-reply tracer.
• docs/reborn/contracts/product-adapters.md not visibly touched in this round — please confirm it's not still describing fake services / v1 exclusivity.

Verdict

Approve. All four critical items from the prior CHANGES_REQUESTED round are now addressed with caller-level tests on the public APIs. The tracer's no-reply property is structurally enforced, not just documented. The two residual String exposures and the getMe-failure placeholder are tracked and acceptable for a default-off tracer; revisit before rollout.

— Zaki

[serrrfirat]

Multi-agent review for #3590 at 2278da82a9602a680a44ba3ea346ce27088fbd22.

Result: requesting changes. I found 2 High issues and 5 Medium issues worth fixing before merge. The High issues are the durable ledger ownership race after recovery-lease reclaim in both storage backends.

Additional non-inline follow-ups:

• Add Postgres contract coverage for ON CONFLICT contention and stale lease recovery, preferably proving the action_id ownership fix in both backends.
• Exercise run_postgres_migrations against a real Postgres pool instead of duplicating a schema constant in tests.
• Remove the stale Reborn Telegram v2 comment left above gateway_base_url in src/extensions/manager.rs.

[serrrfirat]

High: stale owners can overwrite reclaimed actions here. The recovery branch above mints a fresh action_id, but this UPDATE is scoped only by the fingerprint. If the old worker resumes after another request has reclaimed the row, it can settle the new reservation with the old outcome. Scope settle by action_id as well, and make stale settles report a superseded/transient result. The same ownership check is already enforced by the in-memory ledger before moving an in-flight action to settled.

[serrrfirat]

High: stale owners can overwrite reclaimed actions here. The reclaim path replaces the row's action_id, but settle still matches only the fingerprint, so an old worker that resumes after the lease can write its stale outcome onto the new owner's reservation. Include action_id in this WHERE, and apply the same guard to release so stale owners cannot delete a fresh in-flight row.

[serrrfirat]

Medium: false enables these unsafe opt-ins. This treats any non-empty value except 0 as true, so IRONCLAW_REBORN_ALLOW_EPHEMERAL=false enables in-memory storage and IRONCLAW_REBORN_ALLOW_DEFAULT_NAMESPACE=false enables default namespace IDs. Since these are fail-closed escape hatches, parse them strictly: accept only explicit true values such as 1/true, treat empty/0/false as false, and reject any other value.

[serrrfirat]

Medium: the parser says entries with more or fewer than one colon fail closed, but splitn(2, ':') accepts 123:user:extra as a valid pair with user_id = "user:extra". Split on : and require exactly two non-empty parts before installing the pairing, with a regression test for the extra-colon case.

[serrrfirat]

Medium: this always builds the Postgres pool with NoTls. Managed Postgres URLs commonly require TLS via sslmode=require; those deployments will fail during migration/pool connection even though the host advertises Postgres support. Please mirror the existing Postgres TLS connector selection or use a rustls connector for TLS-capable URLs.

[serrrfirat]

Medium: the new required/operational env vars are not reflected in .env.example. This config reads IRONCLAW_REBORN_LISTEN_ADDR, REBORN_TELEGRAM_V2_INSTALLATION_ID, REBORN_TENANT_ID, REBORN_AGENT_ID, TELEGRAM_WEBHOOK_SECRET, REBORN_TELEGRAM_PAIRINGS, IRONCLAW_REBORN_ALLOW_DEFAULT_NAMESPACE, and IRONCLAW_REBORN_ALLOW_EPHEMERAL, while .env.example still only has TELEGRAM_BOT_TOKEN plus the stale REBORN_TELEGRAM_V2_ENABLED block. The repo docs say .env.example lists all env vars, so operators will miss fail-closed settings.

[serrrfirat]

Medium: this expands the v1 AppComponents surface specifically for Reborn Telegram v2 storage, but this PR also documents that Reborn product-layer channels live in ironclaw-reborn and that the v1 agent has zero Reborn wiring or shared in-process state. I also only find this field assigned, not read. Please remove it unless there is a current non-Reborn v1 consumer; the Reborn storage handles should stay inside the standalone host composition.

[serrrfirat]

why are we mingling with app.rs again...

[serrrfirat]

loop landed

[serrrfirat]

Follow-up architecture summary after the inline review:

The line-level review already requested changes for the concrete blockers. Pulling the findings together, I think this PR currently violates several Reborn storage/host-boundary rules and should be reshaped before merge.

1. Idempotency ledger bypasses the universal filesystem + CAS contract.
   Current reborn-integration has ironclaw_filesystem::RootFilesystem::put/get with CasExpectation::{Absent, Version}, and crates/ironclaw_filesystem/CLAUDE.md explicitly says CAS is the floor for claim/consume/transition operations. This PR instead adds backend-specific product_inbound_actions SQL implementations for libSQL and Postgres. The ledger claim/reclaim/settle/release state machine is exactly the kind of multi-step store operation that should be represented as filesystem records and transitioned with CAS + retry on VersionMismatch, unless there is a documented exemption.

2. The direct SQL ledger already produced a correctness bug.
   The inline review flagged that stale workers can settle/delete a reclaimed row because settle and release match only the fingerprint, not the current action_id. A filesystem CAS design would naturally make the version/owner check explicit for every transition.

3. Egress is adapter-mediated but not host-api mediated.
   The adapter itself correctly accepts &dyn ProtocolHttpEgress and a credential handle, but the host implements a bespoke TelegramHttpEgress around reqwest rather than using ironclaw_host_api::RuntimeHttpEgress / host network policy surfaces. Boot-time getMe is also a direct reqwest call.

4. Secrets are handles at the adapter boundary, but not through the host secret surface.
   TELEGRAM_BOT_TOKEN starts as SecretString, then is exposed into a StaticCredentialResolver as a plain String. That means the adapter does not see the raw token, but the host composition is not using ironclaw_host_api::SecretHandle or a real brokered secret resolver.

5. Filesystem usage is mixed.
   Conversation binding and outbound delivery state are routed through ScopedFilesystem, RebornFilesystemConversationServices, and FilesystemOutboundStateStore, which is the right shape. The exception is the idempotency ledger, which remains direct SQL despite the universal FS/CAS API existing on the base branch.

6. Other concrete review findings still apply.
   The earlier review also flagged fail-open env flag parsing (false enabling unsafe opt-ins), malformed pairing parsing with extra colons, Postgres NoTls for TLS-required databases, missing env docs in .env.example, and unused v1 AppComponents.database_handles surface expansion.

Suggested direction: move the idempotency ledger to a filesystem-backed record store using RootFilesystem::put/get and CasExpectation; keep the existing conversation/outbound filesystem path; and either wire egress/secrets through the intended host-api surfaces now or explicitly document why this tracer slice is temporarily allowed to use a bespoke host shim.

[nickpismenkov]

Multi-agent review for #3590 at 10c98777d4e56a6616e0479357d3ad76f172c3bd (post-fix commit). Re-run after the prior review's findings landed in commit 10c98777d.

Verdict: still some High issues to address. 4 High (1 critical-tier rule violation, 1 untested TLS branch, 2 retry-classification bugs that don't fire today but will when outbound lands), 11 Medium, 15 Low/Nit. Posting 15 inline plus a body-only digest for the rest.

What got fixed vs the previous review

• The 3× per-egress block_on_secret is now batched into a single call (was: thread+runtime built 3× per request). ✅
• UrlPath fail-closed branches now have empty-placeholder / control-char / invalid-URL regression tests. ✅
• HostMediatedTelegramEgress now has a contract test suite (host_egress_contract.rs). ✅
• duplicate_update_replays_through_ledger strengthened with action_id stability + fresh-fingerprint cross-checks, plus a directory-listing row-count test in ledger_filesystem_contract.rs. ✅

What's still flagged

High

#	File:Line	Title
1	boot.rs:139	NonZeroUsize::new(64).expect("64 > 0") violates CLAUDE.md's no-panic rule (Critical-tier convention) — use a const evaluator
2	lib.rs:131	Postgres sslmode branching has zero test coverage — a regression flipping the match would silently downgrade TLS on managed Postgres
3	host_egress.rs:252	Response → Network classifies leak-detector matches as retryable; permanent failures will retry forever when outbound lands
4	host_egress.rs:243	Credential → UnknownCredentialHandle puts free-text into a handle field and makes transient StoreUnavailable permanent

Medium (highlights)

File:Line	Issue
config.rs:252	LIBSQL_PATH=:memory: set explicitly bypasses the ALLOW_EPHEMERAL opt-in — fail-closed guard is incomplete
host_runtime/lib.rs:1075	block_on_secret still spawns one OS thread + builds a fresh tokio runtime per request — cache via OnceLock
webhook_e2e.rs:413	Auth tests assert only status codes, never that subtle::ConstantTimeEq is actually wired
inbound_turn.rs:48	StubInboundTurnService has no direct test — UnsupportedActionKind reject branch and v2: accepted-ref prefix are unverified
webhook_e2e.rs:351	Atomic create-or-bind never exercised under concurrent webhooks for the same actor
host_egress_contract.rs:134	New happy-path test doesn't assert response_body_limit (4 MB) or timeout_ms (30 s) on the captured request — load-bearing limits unprotected
host_egress.rs:224	spawn_blocking + inner block_on_secret = two thread hops per outbound; will become a load-bearing cost when reply path wires
inbound_turn.rs:64	Stub hardcodes route_kind: Direct — Telegram group/channel inbound silently mis-binds to direct conversations
ledger_filesystem.rs:211	CAS retry loop matches via reason.contains("version") — fragile across backend driver text and a future error-message rename
composition.rs:61	Bot token downgraded SecretString → String between boot and composition — defeats zeroize-on-drop for ~50 lines
host_egress.rs:246	Request → PolicyDenied conflates composition config errors with network-policy denials (audit noise; not retry-affecting today)
ledger_filesystem.rs:158	begin_or_replay always does read+write — old SQL impl was INSERT-first (1 round-trip on happy path). 4 round-trips per inbound on Postgres-backed FS
webhook_e2e.rs:467	!status.is_success() is too loose — a 5xx regression on the unpaired-actor path would pass (causing Telegram retry storms)
ledger_filesystem_contract.rs:259	Retry-budget exhaustion path (MAX_CAS_RETRIES = 5) untested — dead-code-equivalent without a regression test

Low / Nit (body-only — not posted inline to save the 15-comment budget)

• router.rs:37 — 404 vs 401 lets an unauthenticated attacker enumerate installation_ids
• boot.rs:182 — fetch_bot_identity embeds raw token in a format! heap String (defeats SecretString for the getMe probe)
• host_egress.rs:204 — request.body().to_vec() copies the entire body per send (loses Bytes ref-count; matters for media uploads)
• host_egress.rs:197-215 — network_policy, scope, capability_id, placeholder all cloned per send() — wrap in Arc
• lib.rs:121-147 — Postgres pool uses deadpool defaults (no explicit max_size / timeouts)
• host_runtime/lib.rs:1148 — UrlPath substitution scans then re-parses the URL on every request
• ledger_filesystem.rs:343 — ledger_path() recomputes SHA-256 + JSON serialize per CAS retry
• runtime_http_egress_contract.rs:869+ — new UrlPath tests miss non-ASCII placeholder and non-control-but-URL-unsafe chars
• host_egress_contract.rs:280-319 — unsupported-method test only covers PUT; PATCH/DELETE/lowercase post all share one wildcard arm
• ledger_filesystem_contract.rs:514 — ledger_path collision-resistance / length-bound not asserted directly
• webhook_e2e.rs:572 — duplicate_pairing_install_is_idempotent only proves build_router doesn't error, not that state is uncorrupted
• boot.rs:83-105 — fetch_bot_identity failure-path fallback (bot_user_id=0 sentinel) untested + scrub()'s token redaction unverified
• config.rs:245-276 — resolve_storage precedence not driven through the caller; only the env_flag_set helper is tested
• ledger_filesystem_contract.rs — no crash / partial-write / corrupt-JSON recovery test (intent flagged this risk area explicitly)
• inbound_turn.rs:50 — format!("{:?}", envelope.payload()) may leak user-controlled message content into typed error / logs

Themes

1. map_runtime_error in host_egress.rs:241-256 is one function with three retry-classification bugs (Highs #3, #4 and Medium "Request→PolicyDenied"). The fix is the same: branch on concrete RuntimeHttpEgressError sub-variants and map each to the matching ProtocolHttpEgressError (retryable vs permanent vs timeout). All three resolve together. None fire today (no outbound), so this is "before outbound lands" not "stop ship."
2. Test-through-the-caller gaps are the dominant Medium category — the new code paths (sslmode branching, stub turn service boundary, atomic create-or-bind race, response_body_limit forwarding, ledger retry-budget) all have unit tests on neighboring helpers but nothing that drives the call site. Each is a one-test fix.
3. CLAUDE.md / types.md hygiene — the .expect("64 > 0") and SecretString → String downgrade are both rule-tier violations with one-line fixes (const evaluator, change field type).
4. block_on_secret thread+runtime churn is now 1× per egress (was 3× pre-fix) but still 1×. Worth caching the runtime in a OnceLock before any throughput rollout.

The PR scope ("inbound tracer, no reply") makes most of these latent — the map_runtime_error retry bugs don't fire because outbound isn't wired, and the perf overhead is invisible at tracer volume. But all four Highs become live the moment the reply path lands; better to fix them in this PR than as a follow-up.

🤖 Generated with Claude Code

[nickpismenkov]

🔴 High — Conventions / Critical-tier rule · .expect("64 > 0") violates CLAUDE.md's absolute "No .unwrap() or .expect() in production code" rule. The literal 64 is provably non-zero so the panic is unreachable, but the rule is enforced everywhere else in the codebase. Fix: make it a const:

const RUNNER_CAPACITY: NonZeroUsize = match NonZeroUsize::new(64) {
    Some(n) => n,
    None => unreachable!(),
};

Then pass RUNNER_CAPACITY to the runner config. Compile-time validation, no panic in the binary.

[nickpismenkov]

🔴 High — Tests · Postgres TLS sslmode branching has zero test coverage. The match arm classifies SslMode::Disable → NoTls vs everything else → rustls. A regression flipping the arms (or dropping the _ catch-all) would silently downgrade TLS for managed Postgres (sslmode=require) — and managed Postgres is the production target.

Fix: extract fn ssl_mode_for_url(url: &str) -> Result<SslMode, HostError> and unit-test each branch: ?sslmode=disable, ?sslmode=require, no sslmode (→ Prefer default), and a malformed URL (→ HostError::Storage).

[nickpismenkov]

🔴 High — Bugs · RuntimeHttpEgressError::Response → ProtocolHttpEgressError::Network makes leak-detector matches and response-body-limit-exceeded errors retryable. The host runtime emits Response for permanent response-side failures (response_leak_blocked, decode errors). The adapter retry classifier then treats Network as retryable → the workflow replays a request that will deterministically fail again with the same leak-detector hit.

Doesn't fire today (reply path is stubbed) but lights up the moment outbound lands. Fix:

RuntimeHttpEgressError::Response { reason, .. } => ProtocolHttpEgressError::PolicyDenied {
    reason: RedactedString::new(reason),
},

[nickpismenkov]

🔴 High — Bugs · Credential { reason } → UnknownCredentialHandle { handle: reason } has two problems:

1. reason is free-form sanitized text ("credential store unavailable", "credential lease was already used"); stuffing it into the handle field — which downstream code logs/compares as a credential identity — produces nonsense like egress credential handle 'credential store unavailable' is unknown.
2. UnknownCredentialHandle maps to EgressDenied (not retryable). A transient StoreUnavailable thus becomes permanent and the inbound event is dropped instead of retried.

Fix: add a CredentialUnavailable { reason: RedactedString } variant to ProtocolHttpEgressError (or route through PolicyDenied with the redacted reason). Reserve UnknownCredentialHandle for the path where the runtime actually says the handle id is missing.

[nickpismenkov]

🟡 Medium — Security / fail-closed bypass · The IRONCLAW_REBORN_ALLOW_EPHEMERAL opt-in (lines 260-269) only gates the implicit fallback. An operator who sets LIBSQL_PATH=:memory: directly hits line 253 first and returns LibSql{path: ":memory:"} — connect_backend then routes that to libsql::Builder::new_local(":memory:") without requiring the opt-in.

The in-file comment specifically calls out that silent :memory: is dangerous ("would break the idempotency contract on every restart without anyone noticing") — but a typo or copy-from-test-config triggers it without the warning the ALLOW_EPHEMERAL path emits.

Fix: inspect the path value:

if let Some(path) = env_lookup("LIBSQL_PATH") {
    if path == ":memory:" || path.starts_with(":memory:?") {
        // require ALLOW_EPHEMERAL opt-in just like the implicit fallback
    }
    return Ok(StorageBackend::LibSql { path });
}

[nickpismenkov]

🟡 Medium — Performance · tokio::task::spawn_blocking holds one blocking-pool thread for the entire round-trip (including remote Telegram latency), and the inner block_on_secret spawns another OS thread + runtime. Two thread hops + one runtime build per send. At tracer volume this is invisible, but the default blocking pool is 512 threads and burst webhooks for high-volume bots will starve other spawn_blocking consumers.

Fix path: make HostHttpEgressService::execute async fn and let NetworkHttpEgress/SecretStore stay async end-to-end; the spawn_blocking goes away entirely. Interim: pin secret/IO to a long-lived dedicated runtime so each request is one channel send.

[nickpismenkov]

🟡 Medium — Bugs · ResolveBindingRequest hardcodes route_kind: ProductConversationRouteKind::Direct regardless of envelope content. Telegram group/channel messages must bind under Shared (see DefaultInboundTurnService::route_kind_for_user_message). With Direct, the binding service will either (a) silently merge multi-party group inbound under one direct conversation or (b) reject it on the Direct invariant. The PR title says "Telegram v2 inbound tracer" — a single group message lands on the wrong canonical thread.

Fix: either compute route_kind from envelope.payload().trigger() now (mirroring DefaultInboundTurnService), or reject group-shaped inbound at this boundary with an UnsupportedActionKind that names the missing Shared support — don't silently downgrade.

[nickpismenkov]

🟡 Medium — Bugs / fragile error matching · The CAS retry loops at lines 211, 230, 289 all match reason.contains("version") to detect VersionMismatch. map_fs_error (line 369) renders that variant as "ledger row version mismatch" but renders every other FilesystemError as format!("ledger filesystem error: {other}"). Any future driver text mentioning "version" — "unsupported schema version", "server_version mismatch", even a serde error "missing field 'version'" on a stale row — will be silently treated as a CAS conflict and burn MAX_CAS_RETRIES iterations before surfacing as "contended past retry budget". Real backend failures look like contention.

Fix: match the concrete FilesystemError::VersionMismatch variant in map_fs_error and emit a tagged sentinel (e.g. Transient { reason: "ledger:cas_miss:..." }), then match that exact prefix in the retry loops. Or carry a LedgerError enum through with a typed VersionMismatch arm.

[nickpismenkov]

🟡 Medium — Performance · begin_or_replay always reads then writes. The old SQL impls (now deleted) used INSERT-first with conflict semantics — 1 round-trip in the common (no-prior-row) case. This impl always pays read+write: 2 round-trips per webhook on the happy path, up to 5 read+write iterations under contention. On a Postgres-backed filesystem each round-trip is a real network hop; begin_or_replay + settle ≈ 4 round-trips per inbound. At tracer volume invisible; at reply-path volume meaningful.

Fix: try CasExpectation::Absent put first; only read on VersionMismatch. Happy path drops to one round-trip and matches the old SQL behavior.

[nickpismenkov]

🟡 Medium — Conventions / types.md · HostConfig::telegram_bot_token: SecretString (zeroize + redacted Debug) is downgraded to RebornProductRuntimeConfig::telegram_bot_token: String in boot.rs:75 via expose_secret().to_string(). The plain String then lives on the heap until line ~153 where it's consumed into SecretMaterial::from(...). That window spans secret_store.put(...).await plus storage layer builds — defeating zeroize-on-drop for the busiest part of startup.

Fix: change this field to SecretString (or ironclaw_secrets::SecretMaterial if appropriate) and call .expose_secret() only at the final SecretMaterial::from(...) site. Mirrors how HostConfig::telegram_webhook_secret already stays SecretString end-to-end.

[zmanian]

Re-review (post 7e6ecb607, FS-CAS ledger landing)

Following my prior APPROVE at #3590 (issuecomment-4458499619). Two material changes since: 2278da82a and the 7e6ecb607 merge that pulls in the durable filesystem-CAS ledger (#3759). My four prior major-tier items remain resolved/tracked, and the latest CHANGES_REQUESTED findings (serrrfirat 2026-05-20) are addressed by the FS-CAS migration.

Substrate boundary — verified clean

This PR adds only ironclaw_reborn_telegram_v2_host as a workspace member; root Cargo.toml [dependencies] gains zero Reborn entries; no use ironclaw_reborn:: / use ironclaw_product_:: / use ironclaw_telegram_v2:: anywhere under src/. The v1 binary remains compile-time-unaware. reborn_dependency_boundaries.rs is correctly pinned with the per-channel-host allow-list pattern called out for future channels.

Webhook auth — correct

SharedSecretHeaderAuth (auth_verifier.rs:235) uses subtle::ConstantTimeEq and rejects empty expected_secret as malformed config. Auth runs before parse/workflow in NativeProductAdapterRunner::process_webhook. Router maps AuthenticationFailed to 401.

Ledger correctness — resolved

Migration to FilesystemIdempotencyLedger over RootFilesystem (8c91d75) replaces dual SQL backends with a single CAS-versioned store where the version token is the saga ownership token. settle/release (ledger_filesystem.rs:244,300) explicitly check existing.action_id != action.action_id and surface superseded — closing serrrfirat's CHANGES_REQUESTED High on stale-worker writes after recovery-lease reclaim.

Egress — host-mediated, resolved

HostMediatedTelegramEgress routes through RuntimeHttpEgress / HostHttpEgressService / PolicyNetworkHttpEgress. No bespoke reqwest client survives. RuntimeCredentialTarget::UrlPath is the right primitive for path-embedded bot tokens.

Residuals (non-blocking, all pre-rollout)

Medium — fetch_bot_identity token in URL string (boot.rs:192). scrub(err) strips URLs from reqwest::Error displays, but the URL with the token lives as plain String on the heap between client build and .send(). Defeats SecretString for the getMe path. Recommend merging into #3606 as one pre-rollout follow-up.

Medium — StubInboundTurnService hardcodes route_kind: Direct (inbound_turn.rs:127). Group/channel inbound silently mis-binds until the outbound migration. Acknowledged in code; non-blocking only because the stub produces no reply.

Medium — bot_user_id = 0 placeholder on getMe failure (boot.rs:115). Group-trigger classifier accepts 0; transient startup network errors silently degrade group routing. Consider fail-closed or a metric.

Low — host-owned ledger path layout is implicit between the host crate and ledger_filesystem.rs. A constant-string-comparison test would lock it.

Low — 404-before-401 on unknown installation (router.rs:37) lets an unauthenticated probe enumerate installation IDs.

Verdict

APPROVE. Substrate boundary holds, all CHANGES_REQUESTED resolved, residuals are correctly scoped to pre-rollout follow-ups.

[zmanian]

Review: COMMENT (lean approve for the narrowed inbound-tracer scope).

The PR has been substantially reshaped since the open CHANGES_REQUESTED reviews. The current head no longer contains the dual SQL ledgers (ledger_libsql.rs/ledger_postgres.rs), the per-backend binding files, or the V26/V28 migrations earlier reviewers critiqued — those are replaced by a single FilesystemIdempotencyLedger over the universal RootFilesystem/CAS contract. The verifiable prior blockers appear addressed; final merge gate should be the original reviewers (serrrfirat/henrypark) dismissing their now-stale reviews.

Substrate boundary — clean (the priority concern): root Cargo.toml [dependencies] gains zero Reborn entries; ironclaw_reborn_telegram_v2_host is a workspace member wired into the separate ironclaw-reborn binary only. No inverted dependency. The v1/v2 exclusivity guard removal from main.rs/extensions/manager.rs is correct now that v2 is its own binary. reborn_dependency_boundaries.rs enforces the boundary with an allow-list + a forbidden rule on ironclaw_product_workflow_storage.

Should-fix

• ledger_filesystem.rs (map_fs_error + CAS loops): version-conflict detection round-trips a typed FilesystemError::VersionMismatch into Transient { reason: "...version mismatch" } then re-detects via reason.contains("version"). Lossy string-matching for control flow — carry a typed sentinel/variant instead. In-crate, so not a correctness bug today.
• boot.rs ~192 fetch_bot_identity: getMe builds the URL as a plain String, so the token lives on the heap outside SecretString for this one path, and it's a bespoke reqwest call outside the host egress surface. Already tracked in #3606 — fine to defer, but route it through the host egress like the send path.

Nits

• inbound_turn.rs ~127: stub hardcodes route_kind: Direct; group/channel inbound mis-binds until the outbound migration (acknowledged in code).
• info! logs on the inbound path are fine here — this is the ironclaw-reborn binary, not the v1 REPL/TUI, so the CLAUDE.md info!-corrupts-TUI rule doesn't apply.

Test coverage — strong for a tracer and drives through the caller (webhook POST → workflow → ledger): settle, duplicate-replay, 401 on wrong/missing secret, 404 on unknown installation, unpaired-actor-does-not-settle (BindingRequired regression), plus CAS tests (stale-settle-after-reclaim, fabricated-action-id rejection, concurrent-begin funneling) and host-egress contract tests.

Prior CHANGES_REQUESTED status (predates the FS-CAS reshape): recovery-lease wedge, binding/thread orphan race, default-namespace collision, idempotency-bypasses-CAS, stale-worker settle, egress-not-host-mediated, and the stale extensions/manager.rs comment all appear resolved in the current head. Remaining: getMe heap exposure (#3606). A re-review from serrrfirat/henrypark is what's needed to clear the gate.

[zmanian]

Re-review (head cbe23032f, prior review was 7e6ecb60, 2026-05-23)

What changed since my 2026-05-23 review

Almost nothing in this PR's own surface. The diff between my prior review commit and the current head is dominated entirely by the base branch (reborn-integration) advancing via two merge commits — subagent-spawn docs, trace-CLI refactors, secrets/keychain work, etc. None of that is authored by this PR.

The only PR-authored changes to its own crates since 7e6ecb60 are two small forward-compat adaptations to keep up with new upstream enum variants:

1. crates/ironclaw_telegram_v2_adapter/src/adapter.rs — render_outbound now also matches the new ProductOutboundPayload::CapabilityActivity(_) and ProductOutboundPayload::KeepAlive variants, routing them to the existing "Telegram does not consume this → Deferred + record status" arm. The match remains exhaustive with no wildcard (all 8 variants enumerated), so a future variant fails compilation rather than silently mis-routing — good defensive shape. A focused test (render_outbound_capability_activity_deferred_without_egress) drives the new arm and asserts no egress call fires. .expect() usages are test-only (allowed).

2. crates/ironclaw_reborn_telegram_v2_host/src/host_egress.rs — one line adds the new RuntimeHttpEgressReasonCode::PolicyDenied to the permanent-PolicyDenied arm of map_runtime_error. Correct classification (a policy denial is not retryable).

Status of prior findings

• map_runtime_error retry-classification Highs (nickpismenkov #3 Response→Network-retryable-forever, #4 free-text into UnknownCredentialHandle.handle) — Resolved at this head. The function now switches on a typed reason_code() enum (no string matching), maps ResponseError/ResponseBodyLimitExceeded/CredentialUnavailable/RequestDenied/PolicyDenied all to permanent PolicyDenied, and surfaces only the allow-listed stable_runtime_reason() instead of raw reason text. The doc comment explicitly calls out that this keeps host-internal strings out of the handle field. Both latent retry bugs are closed before the reply path lands.
• Substrate boundary / no inverted dependency — Still clean. Confirmed in my prior review; unchanged. Root Cargo.toml gains no Reborn deps; host is a workspace member wired only into the ironclaw-reborn binary; reborn_dependency_boundaries.rs enforces it.
• No-reply tracer property — Still structurally enforced. No sendMessage; the two new payload arms both terminate in Deferred without touching egress (test-asserted).
• getMe heap token exposure (#3606) — Still open / tracked, unchanged this round. Acceptable to defer for a default-off tracer.
• ledger_filesystem.rs lossy reason.contains("version") CAS control flow — Still open, unchanged. In-crate, not a correctness bug today; carry a typed sentinel when convenient.
• inbound_turn.rs stub hardcodes route_kind: Direct — Still open (acknowledged in code), fires only when outbound migration lands.

New findings

• None at correctness/security severity. The only PR-authored delta is the two adaptations above, both correct.
• Nit (adapter.rs): the shared arm's comment ("Telegram never consumes projection subscriptions") is now slightly inaccurate — CapabilityActivity and KeepAlive aren't projection subscriptions. The Deferred reason string "telegram surface does not consume projection envelopes" is similarly narrow for the two new variants. Cosmetic; the behavior is correct.

Test coverage

Unchanged-and-strong from the prior round (webhook → workflow → ledger: settle, duplicate-replay, 401/404, unpaired-actor, CAS reclaim, host-egress contract). The new variant gets a caller-level test that drives render_outbound and asserts no egress side effect — exactly the "test through the caller" expectation.

Recommendation

COMMENT / lean approve, unchanged from 2026-05-23. There is effectively no new PR-authored risk surface since my last review — just two correct forward-compat patches for new upstream enum variants, one of which (PolicyDenied) plus the surrounding map_runtime_error rework actually closes the prior retry-classification Highs. Residual items (getMe #3606, FS-CAS string match, stub route_kind: Direct) remain deferred and acceptable for a default-off, no-reply tracer. Merge gate remains a dismissal of the now-stale CHANGES_REQUESTED reviews by serrrfirat/henrypark.

Re-reviewed on behalf of @zmanian.