Skip to content

[codex] Wire planned profile resolver in Reborn composition - #3717

Merged
serrrfirat merged 2 commits into
reborn-integrationfrom
reborn/3696
May 18, 2026
Merged

serrrfirat merged 2 commits into
reborn-integrationfrom
reborn/3696

Conversation

@henrypark133

Copy link
Copy Markdown
Collaborator

Summary

Fixes #3696 by wiring the planned Reborn run-profile resolver into production-shaped Reborn composition before building the production turn coordinator.

What changed

  • Added ironclaw_reborn as a composition-root dependency so ironclaw_reborn_composition can use default_planned_run_profile_resolver().
  • Wired the planned resolver into both libSQL and PostgreSQL production/migration service assembly before turn_coordinator_for_production().
  • Added an explicit PlannedRunProfileResolver build error instead of silently falling back to a generic/default resolver.
  • Added caller-level libSQL and PostgreSQL facade tests that submit a turn and assert the resolved profile is reborn-planned-default.

Boundary notes

  • Keeps the fix in ironclaw_reborn_composition, the top-level Reborn composition root.
  • Leaves HostRuntimeServices::turn_coordinator_for_production fail-closed when no resolver is configured.
  • Does not route through legacy bridge paths or expose lower substrate handles.
  • cargo test -p ironclaw_architecture passes, including Reborn composition boundary checks.

Review pass

Ran a read-only subagent review before publishing. It found one valid minor issue: an unconditional InMemoryRunProfileResolver import triggered a no-feature unused-import warning. Addressed by cfg-gating that import.

Validation

  • cargo test -p ironclaw_reborn_composition --test facade_factory
  • cargo test -p ironclaw_reborn_composition --features libsql --test facade_factory migration_dry_run_validates_libsql_shape
  • cargo test -p ironclaw_reborn_composition --features postgres --test facade_factory migration_dry_run_validates_postgres_planned_turn_profile
  • cargo test -p ironclaw_architecture
  • git diff --check

@github-actions github-actions Bot added scope: dependencies Dependency updates size: S 10-49 changed lines risk: low Changes to docs, tests, or low-risk modules contributor: core 20+ merged PRs labels May 17, 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 integrates the ironclaw_reborn planned run-profile resolver into the composition layer for libsql and postgres production backends. It introduces a new error variant for resolver build failures and adds integration tests for Postgres using testcontainers to validate the migration dry-run logic. Review feedback suggests improving error handling by using a more specific error variant with structured logging and recommends wiring the resolver into the local-dev profile to ensure dev/prod parity.

Comment on lines +15 to +16
#[error("reborn planned run-profile resolver build failed: {reason}")]
PlannedRunProfileResolver { reason: String },

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

Instead of manually mapping the error to a string, create a specific error variant for this failure mode. To maintain observability without leaking implementation details, log the detailed error from the dependency using a structured logger (e.g., tracing) at the call site and return a sanitized, generic error variant.

Suggested change
#[error("reborn planned run-profile resolver build failed: {reason}")]
PlannedRunProfileResolver { reason: String },
#[error("reborn planned run-profile resolver build failed")]
PlannedRunProfileResolverBuildFailed,
References
  1. Create specific error variants for different failure modes to provide semantically correct and clear error messages.
  2. When handling errors from external dependencies, log the detailed error using a structured logger for observability, while returning a sanitized, generic error to the caller to avoid leaking implementation details.

})
}

#[cfg(any(feature = "libsql", feature = "postgres"))]

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 planned run-profile resolver is currently only wired for production backends (libsql and postgres). To ensure dev/prod parity, it should also be wired into the local-dev profile (around line 121). This ensures that local development environments use the same Reborn-specific run profiles and drivers as production, avoiding potential discrepancies in behavior.

Copilot AI 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.

Pull request overview

Wires the planned Reborn run-profile resolver (default_planned_run_profile_resolver) into both libSQL and PostgreSQL production composition paths so turn_coordinator_for_production() no longer fails closed in production composition, fixing the regression from #3653.

Changes:

  • Adds ironclaw_reborn as a composition-root dep and a planned_run_profile_resolver() helper that wraps it with a new RebornBuildError::PlannedRunProfileResolver error variant.
  • Calls .with_run_profile_resolver(planned_run_profile_resolver()?) before building the production coordinator in both libSQL and Postgres paths.
  • Adds libSQL and Postgres facade tests that submit a turn and assert resolved_run_profile_id == PLANNED_DEFAULT_PROFILE_ID.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
crates/ironclaw_reborn_composition/Cargo.toml Adds ironclaw_reborn dep and chrono dev-dep for tests.
crates/ironclaw_reborn_composition/src/factory.rs Adds resolver helper and wires it into libSQL/Postgres production builds.
crates/ironclaw_reborn_composition/src/error.rs Adds PlannedRunProfileResolver build error variant.
crates/ironclaw_reborn_composition/tests/facade_factory.rs Adds Postgres testcontainer harness and planned-profile assertions for both backends.
Cargo.lock Reflects new dep edges.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@henrypark133
henrypark133 marked this pull request as ready for review May 17, 2026 05:26

@serrrfirat serrrfirat 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.

I reviewed this in three passes: general correctness, a paranoid/security-correctness pass, and a focused multi-tenancy pass.

I did not find a runtime correctness, security, or tenant-isolation blocker in the resolver wiring. The new production-shaped composition path injects the planned Reborn run-profile resolver before building the production turn coordinator, and the submitted turn path continues to carry full TurnScope isolation through records, active locks, idempotency, DB scope keys, event projection, checkpoint lookup, and admission buckets. The resolver itself is static/global and does not receive tenant context, which is acceptable for the current static planned default profile; if future hosted behavior needs tenant/org-specific profile policy, that should become an explicit resolver contract change rather than hidden inside this PR.

The only blocking item I found is mergeability: the branch currently conflicts with reborn-integration in crates/ironclaw_reborn_composition/Cargo.toml after base branch movement. Please rebase/resolve that conflict and rerun the relevant composition checks before merge.

Validation I ran locally: cargo check -p ironclaw_reborn_composition --no-default-features, cargo check -p ironclaw_reborn_composition --features postgres --tests, cargo test -p ironclaw_reborn_composition --test facade_factory, the targeted libSQL/Postgres planned-profile facade tests, cargo test -p ironclaw_architecture, git diff --check, and git merge-tree origin/reborn-integration HEAD to confirm the merge conflict.

…3696

# Conflicts:
#	crates/ironclaw_reborn_composition/Cargo.toml
@serrrfirat
serrrfirat merged commit c58444d into reborn-integration May 18, 2026
15 checks passed
@serrrfirat
serrrfirat deleted the reborn/3696 branch May 18, 2026 13:37
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
[codex] Wire planned profile resolver in Reborn composition
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: low Changes to docs, tests, or low-risk modules scope: dependencies Dependency updates size: S 10-49 changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants