feat: (product-workflow) Add durable product workflow ledger

[serrrfirat]

Summary

• add libSQL and PostgreSQL-backed ProductWorkflow idempotency ledgers behind the existing IdempotencyLedger port
• persist inbound action reservations/outcomes with recovery-lease semantics for non-terminal actions
• add libSQL contract coverage for reopen replay, in-flight lease blocking/reclaim, and release-before-retry

Validation

• cargo test -p ironclaw_product_workflow
• cargo test -p ironclaw_product_workflow --features libsql
• cargo check -p ironclaw_product_workflow --features postgres
• cargo check -p ironclaw_product_workflow --features "libsql postgres"
• cargo clippy -p ironclaw_product_workflow --all-targets --features "libsql postgres" -- -D warnings

[gemini-code-assist]

Code Review

This pull request introduces durable IdempotencyLedger implementations for libSQL and PostgreSQL to the ironclaw_product_workflow crate, along with associated integration tests and dependencies. The feedback highlights opportunities to optimize performance by ensuring migrations run only once per instance, improve maintainability by using helper functions instead of hardcoded strings in SQL queries, and enhance observability by logging failures during transaction rollbacks.

[gemini-code-assist]

Running migrations on every ledger operation introduces significant overhead, as it requires a roundtrip to the database to check the schema for every begin_or_replay, settle, and release call. Consider running migrations once during application startup or using a synchronization primitive like tokio::sync::OnceCell within the struct to ensure they only run once per instance lifetime.

[gemini-code-assist]

The list of terminal phases is hardcoded as strings in the SQL query. This is fragile and may lead to data loss or incorrect behavior if new terminal phases are added to the ActionPhase enum but not updated here. It is safer to construct the query using the phase_label function to ensure consistency with the rest of the implementation.

        conn.execute(
            &format!(
                "DELETE FROM reborn_product_workflow_actions
             WHERE adapter_id = ?1
               AND installation_id = ?2
               AND source_binding_key = ?3
               AND external_event_id = ?4
               AND action_id = ?5
               AND phase NOT IN ('{}', '{}')",
                phase_label(ActionPhase::Settled),
                phase_label(ActionPhase::DeduplicatedReplay)
            ),
            params![
                action.fingerprint.adapter_id.as_str(),
                action.fingerprint.installation_id.as_str(),
                action.fingerprint.source_binding_key.as_str(),
                action.fingerprint.external_event_id.as_str(),
                action.action_id.to_string(),
            ],
        )

[gemini-code-assist]

When a transaction operation fails and a rollback is attempted, any failure during the rollback itself should be logged as a warning. This prevents secondary failures from being completely silent while still ensuring the original primary error is returned to the caller.

            Err(error) => {
                if let Err(rollback_error) = conn.execute("ROLLBACK", ()).await {
                    tracing::warn!("idempotency ledger failed to rollback transaction: {rollback_error}");
                }
                Err(error)
            }

References

1. When a primary operation fails and a subsequent cleanup or secondary operation also fails, log the secondary failure as a warning and ensure the original primary error is returned to the caller to prevent it from being swallowed.

[gemini-code-assist]

Similar to the libSQL implementation, calling run_migrations on every operation in the PostgreSQL ledger adds unnecessary overhead. Consider centralizing migration execution or using a OnceCell to ensure it only runs once.

[gemini-code-assist]

The terminal phases are hardcoded in the SQL query, which poses a maintenance risk. Construction of the query should ideally use the phase_label function to stay in sync with the enum definitions.

        txn.execute(
            &format!(
                "DELETE FROM reborn_product_workflow_actions
             WHERE adapter_id = $1
               AND installation_id = $2
               AND source_binding_key = $3
               AND external_event_id = $4
               AND action_id = $5
               AND phase NOT IN ('{}', '{}')",
                phase_label(ActionPhase::Settled),
                phase_label(ActionPhase::DeduplicatedReplay)
            ),
            &[
                &action.fingerprint.adapter_id.as_str(),
                &action.fingerprint.installation_id.as_str(),
                &action.fingerprint.source_binding_key.as_str(),
                &action.fingerprint.external_event_id.as_str(),
                &action.action_id.to_string(),
            ],
        )

[gemini-code-assist]

Rollback failures should be logged as warnings to ensure visibility into secondary cleanup errors, while preserving the original error for the caller.

            Err(error) => {
                if let Err(rollback_error) = txn.rollback().await {
                    tracing::warn!("idempotency ledger failed to rollback transaction: {rollback_error}");
                }
                Err(error)
            }

References

1. When a primary operation fails and a subsequent cleanup or secondary operation also fails, log the secondary failure as a warning and ensure the original primary error is returned to the caller to prevent it from being swallowed.

[serrrfirat]

Multi-agent code review for PR #3759 (--force used because the PR is draft).

Summary:

• Security: no findings
• Bugs: no findings
• Performance/Concurrency: 1 finding
• Tests: 3 findings
• Conventions: 1 finding

I would request changes for these findings, but GitHub does not allow requesting changes on my own PR, so this is posted as a review comment.

[serrrfirat]

[Medium][conventions/boundary] ironclaw_product_workflow/AGENTS.md says storage backend details must not move into this crate. This new module embeds SQL schema and libSQL/Postgres implementations directly in product_workflow. Move the concrete storage implementation to the storage/DB-owning layer or a storage adapter crate, and keep this crate behind the existing IdempotencyLedger port.

[serrrfirat]

[High][tests] The PostgreSQL-backed ledger has its own SQL, transaction, FOR UPDATE, lease, settle, and release paths, but the contract test file is cfg(feature = "libsql") only. Add PostgreSQL parity tests covering settled replay across reopen, in-flight lease block/reclaim, and release-before-retry.

[serrrfirat]

[High][tests/concurrency] begin_or_replay relies on database uniqueness/transactions for the core duplicate-reservation race, but the durable tests only make sequential duplicate calls. Add a contention test where two concurrent begin_or_replay calls for the same fingerprint produce exactly one New and one transient/in-flight result.

[serrrfirat]

[Medium][tests] This stale-action mismatch path is important after lease reclaim: the old holder must not be able to settle over the newer reservation. Add a contract test that begins an action, lets the lease expire/reclaims it, then verifies settling the original action returns the superseded-reservation error and does not overwrite the current row.

[serrrfirat]

[Medium][performance/concurrency] Every ledger operation reruns migrations before doing DML. On the product inbound hot path this adds an extra connection and DDL/catalog work per action; for Postgres it can also take DDL-related locks under concurrent traffic. Run migrations at construction/startup or guard them with a per-instance OnceCell/OnceLock so normal ledger operations only perform the transaction they need.

[serrrfirat]

Multi-agent code review completed with --force.

Event: COMMENT

GitHub rejected REQUEST_CHANGES for this account because it owns the pull request, so this is posted as a comment review. Treat the High finding as blocking.

Findings kept after confidence filtering:

• High: 1
• Medium: 4
• Low: 3

Primary blocker:

• cargo test -p ironclaw_product_workflow_storage --features postgres --no-run fails because the postgres-only contract test build uses Arc while the import is gated behind feature = "libsql".

Reviewer lanes: security, bugs, performance/concurrency, tests, conventions.

[serrrfirat]

[High][build] Arc is only imported when feature = "libsql", but the postgres-only test build uses it in the postgres tests and in postgres_filesystem. I verified cargo test -p ironclaw_product_workflow_storage --features postgres --no-run fails with undeclared Arc, so the stated postgres feature combination is broken. Gate this import on any(feature = "libsql", feature = "postgres") or import it unconditionally.

[serrrfirat]

[Low][security] This logs the full backend FilesystemError with %error. Those errors include the virtual path, and the ledger path is built from reversible hex of adapter, installation, source binding, and external event identifiers. On backend failures, logs can expose conversation/event identifiers and backend details. Please log a sanitized category/status here, or make the path components non-reversible before they reach error display.

[serrrfirat]

[Medium][tests] The missing-reservation branch in settle is new behavior, but the contract tests only cover successful settle and superseded reservations. A regression that silently accepts an unreserved action would not be caught. Add libsql_settle_missing_reservation_returns_transient and the postgres *_when_configured equivalent that call settle(ProductInboundAction::begin(...)) before any reservation.

[serrrfirat]

[Low][tests] RebornLibSqlIdempotencyLedger::with_root is public but never exercised by the adjacent contract tests. If this constructor accidentally ignored the supplied root, the existing reopen/replay and lease tests would still pass. Add a libsql_custom_root_isolated_from_default_root contract case.

[serrrfirat]

[Low][tests] RebornPostgresIdempotencyLedger::with_root is public but not covered by the postgres contract path. If it falls back to the default root, configured postgres tests would still pass. Add postgres_custom_root_isolated_from_default_root_when_configured.

[serrrfirat]

[Medium][performance] begin_or_replay runs on every inbound product action, but this durable ledger goes through the generic filesystem put/get path. On the SQL backends each put performs directory/child prechecks before the actual write, and settle/release add a read plus another checked write, so a normal successful action needs several SQL round trips before ack completion. If this is the intended hot-path implementation, consider adding a RootFilesystem-level batch/transition helper or another architecture-compatible fast path before enabling it for high inbound volume.

[serrrfirat]

[Medium][conventions] This adds a new workspace crate, but crates/ironclaw_product_workflow_storage has no crate-local AGENTS.md. crates/AGENTS.md says each crate with a Cargo.toml has local instructions and agents should read them first. Please add crate ownership, boundaries, and validation guidance for this new crate.

[serrrfirat]

[Medium][conventions] FilesystemIdempotencyLedger stores the durable root as a raw String, even though callers provide a validated VirtualPath and filesystem APIs operate on VirtualPath. The typed-internals rule says raw strings are boundary formats; keep this as a VirtualPath internally and format child paths only at the final construction point.

[serrrfirat]

Addressed the review findings in b8c2736:\n\n- fixed the postgres-only contract test compile failure by enabling the Arc import for postgres\n- added libSQL/postgres contract coverage for missing-reservation settle and custom-root isolation\n- kept ledger roots typed as VirtualPath internally\n- sanitized durable ledger error logging so backend paths/details are not logged\n- added a RootFilesystem known-leaf CAS write hook and SQL overrides, then used it from the durable ledger hot path\n- added crate-local AGENTS.md guidance for ironclaw_product_workflow_storage\n\nLocal verification run:\n- cargo test -p ironclaw_product_workflow_storage --features libsql\n- cargo test -p ironclaw_product_workflow_storage --features postgres --no-run\n- cargo test -p ironclaw_product_workflow_storage --features postgres\n- cargo check -p ironclaw_product_workflow_storage --features "libsql postgres"\n- cargo clippy -p ironclaw_product_workflow_storage --all-targets --features "libsql postgres" -- -D warnings\n- cargo test -p ironclaw_filesystem --features "libsql postgres"\n- bash scripts/pre-commit-safety.sh