feat(migration): add offline v1-to-Reborn migration workflow - #5936
serrrfirat wants to merge 19 commits into
Conversation
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds a manifest-driven v1-to-Reborn migration lifecycle with a same-release companion CLI, read-only source inspection, deterministic target writes, onboarding and quarantine gates, Docker packaging, backend support, and operator documentation. Changesv1 migration bridge
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Operator
participant RebornCLI
participant MigrationCompanion
participant V1Snapshot
participant RebornTarget
Operator->>RebornCLI: migrate v1 plan/apply/resume/verify
RebornCLI->>MigrationCompanion: handshake and forward operation
MigrationCompanion->>V1Snapshot: inspect read-only source
MigrationCompanion->>RebornTarget: apply, resume, or verify migration state
RebornCLI->>RebornCLI: enforce activation state
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size: XL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive, multi-stage offline migration workflow to transition state from IronClaw v1 and engine-v2 to Reborn. It adds a same-release migration companion executable (ironclaw-reborn-migration) and implements a structured lifecycle consisting of read-only planning, stopped-source snapshot applying/resuming, and target verification. The changes also include deterministic target identifier generation, user and project migration, and updated onboarding and Docker configurations. The review feedback suggests two key improvements: using injective length-prefixed encoding instead of null-delimited concatenation for deterministic UUID generation to prevent separator-collision attacks, and canonicalizing the executable path in resolve_companion to ensure correct resolution when the binary is invoked via a symlink.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn scoped_uuid( | ||
| &self, | ||
| domain: &str, | ||
| source_primary_id: &str, | ||
| tenant_id: &str, | ||
| agent_id: &str, | ||
| ) -> Uuid { | ||
| let seed = format!( | ||
| "{MIGRATION_ID_SCHEMA}\0{}\0{}\0{domain}\0{source_primary_id}\0{tenant_id}\0{agent_id}", | ||
| self.manifest_schema_version, self.source_fingerprint | ||
| ); | ||
| Uuid::new_v5(&MIGRATION_NAMESPACE, seed.as_bytes()) | ||
| } |
There was a problem hiding this comment.
Generating deterministic UUIDs by concatenating multiple string components with a null delimiter (\0) is vulnerable to separator-collision attacks if any component can contain the delimiter or if the structure changes. To ensure complete injectivity and eliminate collision risks, use an injective length-prefixed encoding instead of simple concatenation.
fn scoped_uuid(
&self,
domain: &str,
source_primary_id: &str,
tenant_id: &str,
agent_id: &str,
) -> Uuid {
let mut seed = Vec::new();
for component in &[
MIGRATION_ID_SCHEMA,
&self.manifest_schema_version.to_string(),
&self.source_fingerprint,
domain,
source_primary_id,
tenant_id,
agent_id,
] {
seed.extend_from_slice(&(component.len() as u64).to_le_bytes());
seed.extend_from_slice(component.as_bytes());
}
Uuid::new_v5(&MIGRATION_NAMESPACE, &seed)
}References
- When generating deterministic identifiers or hashes from multiple string components, use an injective length-prefixed encoding rather than simple concatenation with a delimiter to eliminate the risk of separator-collision attacks or accidental collisions.
- Prefer using
u64for length prefixes in binary encodings to ensure that conversions fromusizeare infallible on all supported platforms, avoiding panics and potential DoS vectors caused by.expect()on overflow.
There was a problem hiding this comment.
Fixed in 5a3bfa2. Deterministic ID seeds now use injective length-prefixed field framing under a new ID schema, and the manifest schema is bumped to v3 so earlier rehearsal manifests cannot be mixed with the new IDs. The regression test constructs the prior null-separator collision and proves the IDs differ.
| fn resolve_companion(current_exe: &Path) -> anyhow::Result<PathBuf> { | ||
| let bin_dir = current_exe.parent().with_context(|| { | ||
| format!( | ||
| "cannot resolve the installation directory for {}", | ||
| current_exe.display() | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
If the running executable is invoked via a symlink (e.g., when installed via stow or custom symlinks in /usr/local/bin), std::env::current_exe() may return the symlink path. Using .parent() on the symlink path will resolve to the directory containing the symlink rather than the directory containing the actual binary, failing to locate the companion. Canonicalizing the path first ensures symlinks are resolved correctly.
fn resolve_companion(current_exe: &Path) -> anyhow::Result<PathBuf> {
let current_exe = current_exe.canonicalize().with_context(|| {
format!(
"failed to canonicalize executable path {}",
current_exe.display()
)
})?;
let bin_dir = current_exe.parent().with_context(|| {
format!(
"cannot resolve the installation directory for {}",
current_exe.display()
)
})?;There was a problem hiding this comment.
No code change by design. This launcher intentionally rejects symlinked companions and install directories writable by another user; canonicalizing the invoked binary to make symlink installs work would weaken that documented same-directory/native-companion trust boundary. Existing migration CLI tests cover missing, cross-release, and writable companion rejection.
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 1 | 2 | c64d03054e5e |
Head: c64d03054e5e2f199d0e65109c74fd9c14175003
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found one blocking migration lifecycle bug and one non-blocking strict-mode bug. Cargo-based validation could not be run because cargo is not installed in the reviewer environment.
Findings
Blocking: 1 / Notes: 1
Blocking findings
1. ❌ [MEDIUM] Resume rejects interrupted applying manifests
Location: crates/ironclaw_reborn_migration/src/lib.rs:152
An interrupted apply leaves the manifest in applying because the CLI persists that state before running converters, and resume explicitly accepts an applying manifest. However this path immediately calls manifest.transition(MigrationStatus::Applying), while MigrationManifest::transition does not allow Applying -> Applying. The first resume attempt therefore fails before replaying converters. Treat an already-applying resume as already transitioned, or make that transition idempotent.
Non-blocking notes (1)
1. 💬 [LOW] Strict planning fails on absent lossy categories
Location: crates/ironclaw_reborn_migration/src/main.rs:586-596
manifest_has_strict_loss flags every archive-only, reauth, or reinstall disposition without checking whether the inventory entry actually exists. The inventory builder emits zero-count entries for all known tables and home artifacts, so plan --strict will fail even when, for example, there are no API tokens or WASM installs. Gate these dispositions on entry.count > 0 while keeping blockers unconditional.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/failed/superseded state while reviewers run.
| let source = source::V1Source::open(&source_options.source).await?; | ||
| let mut target = target::RebornTarget::open(&target_options).await?; | ||
| let mut report = MigrationReport::new(false); | ||
| let mut applied_manifest = manifest.transition(MigrationStatus::Applying)?; |
There was a problem hiding this comment.
This breaks resume from the exact state left by an interrupted apply. The CLI writes applying before converter execution, but resume_migration then tries to transition that applying manifest to applying again, which MigrationManifest::transition rejects. Treat already-applying resumes as already transitioned, or allow the transition to be idempotent.
There was a problem hiding this comment.
Already addressed before 5a3bfa2 and re-verified here. The resume path preserves an on-disk Applying manifest instead of attempting Applying -> Applying. Both full migration backend suites pass at the pushed head.
| } | ||
|
|
||
| fn manifest_has_strict_loss(manifest: &MigrationManifest) -> bool { | ||
| manifest.inventory.iter().any(|entry| { |
There was a problem hiding this comment.
This should ignore zero-count lossy dispositions. The planner emits entries for every known source category even when absent, so --strict currently fails on missing API-token/WASM/archive-only categories in otherwise clean sources.
There was a problem hiding this comment.
Fixed in 5a3bfa2. Strict mode now treats a lossy disposition as failure only when entry.count > 0, while any blocker remains unconditional. A caller-level companion test runs a real local-dev/libSQL plan --strict against an empty source and verifies success plus manifest creation.
|
🚅 Deployed to the ironclaw-pr-5936 environment in ironclaw-ci-preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_cli/src/commands/doctor.rs`:
- Around line 44-46: Replace the silent `.ok()?` handling with explicit
propagation or add a `// silent-ok: <reason>` annotation only where fallback is
intentionally valid. Apply this to `migration_state` in
`crates/ironclaw_reborn_cli/src/commands/doctor.rs` lines 44-46,
`status_from_document` in `crates/ironclaw_reborn_cli/src/commands/doctor.rs`
lines 60-62, and `read_target_state_status` in
`crates/ironclaw_reborn_cli/src/commands/migrate.rs` lines 240-242; ensure each
annotation names the specific read if retaining silent fallback.
In `@crates/ironclaw_reborn_cli/src/commands/migrate.rs`:
- Around line 225-268: Introduce a canonical wire-facing migration lifecycle
enum in the migration contract, covering all statuses and providing an
is_activation_safe()/is_quarantined() policy helper. In
crates/ironclaw_reborn_cli/src/commands/migrate.rs lines 225-268, update
ensure_activation_allowed and read_target_state_status to deserialize and use
the shared enum instead of repeating status literals; in
crates/ironclaw_reborn_cli/src/commands/doctor.rs lines 63-72, parse through the
same enum and remove its duplicated accepted-status list.
In `@crates/ironclaw_reborn_cli/src/commands/onboard.rs`:
- Around line 67-81: Replace the string values assigned to migration_state with
a dedicated enum representing explicitly_skipped, planned, available, and
not_detected. Update the match sites around the onboarding summary and
pending_steps to match enum variants, including any formatting or serialization
needed for existing output, while preserving current behavior.
In `@crates/ironclaw_reborn_composition/src/input.rs`:
- Around line 924-936: Update resolve_postgres_migration_target to return the
named ResolvedPostgresTargetConfig instead of a positional tuple, and make that
type pub(crate) so callers can access its named url and secret_master_key
fields. Preserve the existing resolve_postgres_target_config resolution and
error propagation while removing the tuple destructuring.
In `@crates/ironclaw_reborn_migration/src/inventory.rs`:
- Around line 596-602: The checksum assertion around content_digest is
tautological because its prefixes differ and therefore cannot detect content
hashing. Replace it with a meaningful invariant test: create two files with
identical size and mtime but different contents and assert their manifest
checksums match, or remove this assertion and retain only the existing
metadata-independence check.
In `@crates/ironclaw_reborn_migration/src/lib.rs`:
- Line 152: The resume path in resume_migration must avoid rejecting an on-disk
manifest already in MigrationStatus::Applying. Before calling
MigrationManifest::transition(MigrationStatus::Applying), preserve the existing
Applying manifest unchanged; only transition manifests in other permitted resume
states, while retaining the existing applied_manifest flow for newly applying
migrations.
- Around line 445-449: The Postgres branch in the redacted store descriptor
currently hashes the full secret URL, exposing credentials to offline guessing.
Update the locator fingerprint logic around SourceDb handling and locator_hash
to hash only non-secret Postgres locator components, or use the target master
key for an HMAC, while preserving the MigrationManifest redacted
locator-fingerprint contract.
In `@crates/ironclaw_reborn_migration/src/main.rs`:
- Around line 585-597: Update manifest_has_strict_loss to consider an entry
lossy only when entry.count > 0, while preserving blocker handling and the
existing lossy disposition checks for non-empty categories.
In `@crates/ironclaw_reborn_migration/src/source.rs`:
- Line 188: Update the schema lookup return path in the source backend to
propagate row decoding failures instead of converting them to None via
ok().flatten(). Map the try_get error through the existing
source_read_error("schema", error) mechanism, preserving the successful optional
schema-version result and parity with the libSQL path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bdb9cdba-6d30-4cad-8df1-0f5b42c51357
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (54)
Dockerfile.rebornFEATURE_PARITY.mdREADME.mdcrates/ironclaw_reborn_cli/Cargo.tomlcrates/ironclaw_reborn_cli/src/commands/doctor.rscrates/ironclaw_reborn_cli/src/commands/migrate.rscrates/ironclaw_reborn_cli/src/commands/mod.rscrates/ironclaw_reborn_cli/src/commands/onboard.rscrates/ironclaw_reborn_cli/src/context.rscrates/ironclaw_reborn_cli/tests/migration_cli.rscrates/ironclaw_reborn_cli/tests/smoke.rscrates/ironclaw_reborn_composition/src/admin_user_directory.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/input.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/migration_support.rscrates/ironclaw_reborn_identity/CONTRACT.mdcrates/ironclaw_reborn_identity/src/filesystem_store/directory.rscrates/ironclaw_reborn_identity/src/filesystem_store/tests.rscrates/ironclaw_reborn_identity/src/lib.rscrates/ironclaw_reborn_identity/src/user_directory.rscrates/ironclaw_reborn_migration/CLAUDE.mdcrates/ironclaw_reborn_migration/Cargo.tomlcrates/ironclaw_reborn_migration/src/convert/automations.rscrates/ironclaw_reborn_migration/src/convert/extensions.rscrates/ironclaw_reborn_migration/src/convert/memory.rscrates/ironclaw_reborn_migration/src/convert/mod.rscrates/ironclaw_reborn_migration/src/convert/projects.rscrates/ironclaw_reborn_migration/src/convert/secrets.rscrates/ironclaw_reborn_migration/src/convert/threads.rscrates/ironclaw_reborn_migration/src/convert/users.rscrates/ironclaw_reborn_migration/src/inventory.rscrates/ironclaw_reborn_migration/src/lib.rscrates/ironclaw_reborn_migration/src/main.rscrates/ironclaw_reborn_migration/src/manifest.rscrates/ironclaw_reborn_migration/src/mounts.rscrates/ironclaw_reborn_migration/src/options.rscrates/ironclaw_reborn_migration/src/report.rscrates/ironclaw_reborn_migration/src/source.rscrates/ironclaw_reborn_migration/src/target.rscrates/ironclaw_reborn_migration/src/target_ids.rscrates/ironclaw_reborn_migration/src/v2_model.rscrates/ironclaw_reborn_migration/tests/companion_cli.rscrates/ironclaw_reborn_migration/tests/migration_roundtrip.rscrates/ironclaw_reborn_migration/tests/migration_safety.rscrates/ironclaw_reborn_migration/tests/project_migration.rsdocker/reborn/entrypoint.shdocs/internal/2026-06-26-legacy-vs-reborn-feature-comparison.mddocs/reborn-binary.mddocs/reborn/README.mddocs/reborn/deploy-reborn-cli-docker.mddocs/reborn/onboarding.mddocs/reborn/v1-migration.mdsrc/db/libsql/mod.rs
| let mut content_digest = Fnv1a64::new(); | ||
| content_digest.update(canary); | ||
| assert_ne!( | ||
| checksum, | ||
| format!("fnv1a64:{:016x}", content_digest.finish()), | ||
| "manifest checksums must never be hashes of secret-bearing file contents" | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This assertion is tautological and does not test the invariant. checksum is prefixed metadata-fnv1a64: while the comparison target is fnv1a64:…, so assert_ne! always passes regardless of whether contents were hashed. Exercise the actual invariant instead, e.g. two files with identical size/mtime but different contents must produce the same checksum (or drop this weak assertion and keep only the metadata-independence check).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_migration/src/inventory.rs` around lines 596 - 602,
The checksum assertion around content_digest is tautological because its
prefixes differ and therefore cannot detect content hashing. Replace it with a
meaningful invariant test: create two files with identical size and mtime but
different contents and assert their manifest checksums match, or remove this
assertion and retain only the existing metadata-independence check.
There was a problem hiding this comment.
Already addressed before 5a3bfa2 and re-verified at the pushed head. home_checksum_is_not_derived_from_secret_file_contents now creates same-size/same-mtime files with different contents and asserts equal metadata-only checksums; the tautological prefix comparison is gone.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd inventory.rs crates/ironclaw_reborn_migration/src | xargs sed -n '540,610p'Length of output: 2898
🏁 Script executed:
#!/bin/bash
fd inventory.rs crates/ironclaw_reborn_migration/src | xargs grep -n "home_checksum_is_not_derived_from_secret_file_contents" -A 40Length of output: 1870
@serrrfirat, checked the pushed head — the test still contains the same tautological assertion I flagged, not the fix you described.
checksum is prefixed "metadata-fnv1a64:..." while the comparison target is built as format!("fnv1a64:{:016x}", ...) (no metadata- prefix). These two strings can never be equal regardless of whether content is hashed, so assert_ne! is vacuous — it doesn't exercise the invariant at all.
There's no same-size/same-mtime-different-content test present; the only test is home_checksum_is_not_derived_from_secret_file_contents at Line 651-667, unchanged from what I originally flagged.
Possibly you're referencing a local branch state or a different commit than what's currently pushed. Can you confirm the commit SHA you tested against, or push the update if it's still pending?
c64d030 to
3d3cd5a
Compare
github-actions
Bot
added
scope: setup
serrrfirat
changed the title
3d3cd5a to
e695c80
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_cli/src/commands/doctor.rs (1)
1-1: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSilent error handling throughout the migration state detection chain. The
migration_state→status_from_documentpath discards errors at three sites, masking permission-denied, corruption, and parse failures behind opaque fallback strings.
crates/ironclaw_reborn_cli/src/commands/doctor.rs#L255-257: bind theErrinread_activation_state_statusand log withdebug!before returning"invalid".crates/ironclaw_reborn_cli/src/commands/doctor.rs#L260-261: replace.ok()?on the onboarding-marker read/parse with explicitNotFound-aware propagation or add// silent-ok: <reason>naming the onboarding-marker read.crates/ironclaw_reborn_cli/src/commands/doctor.rs#L276-277: same for the manifest read/parse instatus_from_document.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_cli/src/commands/doctor.rs` at line 1, Update the migration-state detection chain in migration_state and status_from_document to stop silently discarding errors. In read_activation_state_status, bind the activation-state read error, log it with debug!, then return "invalid"; replace the onboarding-marker .ok()? handling with explicit NotFound-aware propagation or a reasoned silent-ok annotation, and apply the same treatment to manifest read/parse errors in status_from_document.Sources: Coding guidelines, Path instructions
♻️ Duplicate comments (1)
crates/ironclaw_reborn_cli/src/commands/doctor.rs (1)
260-261: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
.ok()?on IO/parse reads still present — previously flagged.Lines 260–261 (
migration_state) and 276–277 (status_from_document) use.ok()?onstd::fs::read_to_stringandserde_json::from_str, dropping permission-denied and corruption errors intoNone. No// silent-ok: <reason>annotation is present. This is the same fail-loud violation flagged in the prior review; the patterns persist in the reorganized code.Also applies to: 276-277
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_cli/src/commands/doctor.rs` around lines 260 - 261, Update migration_state and status_from_document to stop converting file-read and JSON-parse failures into None via .ok()?. Propagate or explicitly handle the std::fs::read_to_string and serde_json::from_str errors according to the surrounding command error flow, or add the required // silent-ok: <reason> annotation only if intentionally suppressing them.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_cli/src/commands/doctor.rs`:
- Around line 255-257: Update the Err arm handling read_activation_state_status
to bind the error, log it with debug! including relevant context, then return
the existing "invalid" fallback. Preserve the Ok(None) behavior and the current
status output.
---
Outside diff comments:
In `@crates/ironclaw_reborn_cli/src/commands/doctor.rs`:
- Line 1: Update the migration-state detection chain in migration_state and
status_from_document to stop silently discarding errors. In
read_activation_state_status, bind the activation-state read error, log it with
debug!, then return "invalid"; replace the onboarding-marker .ok()? handling
with explicit NotFound-aware propagation or a reasoned silent-ok annotation, and
apply the same treatment to manifest read/parse errors in status_from_document.
---
Duplicate comments:
In `@crates/ironclaw_reborn_cli/src/commands/doctor.rs`:
- Around line 260-261: Update migration_state and status_from_document to stop
converting file-read and JSON-parse failures into None via .ok()?. Propagate or
explicitly handle the std::fs::read_to_string and serde_json::from_str errors
according to the surrounding command error flow, or add the required //
silent-ok: <reason> annotation only if intentionally suppressing them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bc184b66-06e2-4898-9d86-4b9ebae5a5c4
⛔ Files ignored due to path filters (2)
CHANGELOG.mdis excluded by!CHANGELOG.mdCargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (13)
Dockerfile.rebornFEATURE_PARITY.mdREADME.mdcrates/AGENTS.mdcrates/README.mdcrates/ironclaw_reborn_cli/AGENTS.mdcrates/ironclaw_reborn_cli/Cargo.tomlcrates/ironclaw_reborn_cli/src/commands/doctor.rscrates/ironclaw_reborn_cli/src/commands/extension.rscrates/ironclaw_reborn_cli/src/commands/migrate.rscrates/ironclaw_reborn_cli/src/commands/mod.rscrates/ironclaw_reborn_cli/src/commands/onboard.rscrates/ironclaw_reborn_cli/src/context.rs
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.2% — 303641 / 356399 lines Per-crate breakdown (63 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/ironclaw_reborn_cli/src/commands/onboard.rs (1)
71-109: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winContradictory
--migrate-v1+--skip-v1-migrationsilently resolved without feedback.When both flags are set,
skip_v1_migrationwins with no diagnostic, unlike the existing--import-historydeprecation warning right above it. A user who genuinely intended--migrate-v1gets silently skipped.🩹 Proposed fix
- let outcome = write_default_config_files(home, self.force, ExistingConfigPolicy::Preserve)?; - let migration_state = if self.skip_v1_migration { + let outcome = write_default_config_files(home, self.force, ExistingConfigPolicy::Preserve)?; + if self.skip_v1_migration && migration_requested { + eprintln!( + "warning: --skip-v1-migration overrides --migrate-v1/--import-history; v1 migration will not be planned" + ); + } + let migration_state = if self.skip_v1_migration { OnboardingMigrationState::ExplicitlySkipped🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_cli/src/commands/onboard.rs` around lines 71 - 109, In the onboarding flag-handling flow, add an explicit diagnostic when both migrate_v1 and skip_v1_migration are set, before the migration_state selection. Preserve the existing precedence where skip_v1_migration produces ExplicitlySkipped, while clearly informing the user that --migrate-v1 was ignored.crates/ironclaw_reborn_migration/src/source.rs (1)
104-119: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
.map_err(|_| ...)fully discards the Postgres connection error — log before redacting.Redacting the error message to callers is correct (avoids leaking the connection string), but the closure drops the bound error entirely instead of logging it internally first. As per coding guidelines,
"Do not use .map_err(|_| OtherError) or any closure that discards its error binding and substitutes a generic error; carry the cause instead or log the bound error before mapping."This also leaves the Postgres branch inconsistent with the libSQL branch just above it, which carries the cause viasource_open_error.🪵 Proposed fix
let backend = ironclaw::db::postgres::PgBackend::new(&config) .await - .map_err(|_| { + .map_err(|error| { + tracing::debug!(error = %error, "v1 postgres source connection failed"); MigrationError::OpenSource( "PostgreSQL source connection failed (connection details redacted)" .to_string(), ) })?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_migration/src/source.rs` around lines 104 - 119, Update the PostgreSQL backend creation in the SourceDb::Postgres branch to retain and log the underlying error before converting it to the redacted MigrationError::OpenSource message. Replace the discard binding in the PgBackend::new(...).await map_err closure with a named error binding and follow the existing source_open_error or equivalent logging pattern used by the libSQL branch, while preserving redacted caller-facing details.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_migration/src/lib.rs`:
- Around line 40-46: The manifest_target_matches function silently converts
target_descriptor errors into a normal mismatch. Propagate the target_descriptor
error to callers by changing the function’s return type and updating its call
sites, or retain the false fallback only with an inline silent-ok annotation and
explicit error logging; preserve the existing backend and locator_fingerprint
comparisons.
---
Outside diff comments:
In `@crates/ironclaw_reborn_cli/src/commands/onboard.rs`:
- Around line 71-109: In the onboarding flag-handling flow, add an explicit
diagnostic when both migrate_v1 and skip_v1_migration are set, before the
migration_state selection. Preserve the existing precedence where
skip_v1_migration produces ExplicitlySkipped, while clearly informing the user
that --migrate-v1 was ignored.
In `@crates/ironclaw_reborn_migration/src/source.rs`:
- Around line 104-119: Update the PostgreSQL backend creation in the
SourceDb::Postgres branch to retain and log the underlying error before
converting it to the redacted MigrationError::OpenSource message. Replace the
discard binding in the PgBackend::new(...).await map_err closure with a named
error binding and follow the existing source_open_error or equivalent logging
pattern used by the libSQL branch, while preserving redacted caller-facing
details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 05d6e5a7-3c22-475f-bb3f-d2aab1accf9c
📒 Files selected for processing (12)
crates/ironclaw_reborn_cli/src/commands/doctor.rscrates/ironclaw_reborn_cli/src/commands/migrate.rscrates/ironclaw_reborn_cli/src/commands/onboard.rscrates/ironclaw_reborn_composition/src/input.rscrates/ironclaw_reborn_composition/src/migration_support.rscrates/ironclaw_reborn_migration/src/lib.rscrates/ironclaw_reborn_migration/src/main.rscrates/ironclaw_reborn_migration/src/manifest.rscrates/ironclaw_reborn_migration/src/source.rscrates/ironclaw_reborn_migration/src/target_ids.rscrates/ironclaw_reborn_migration/tests/companion_cli.rsdocs/reborn/v1-migration.md
5a3bfa2 to
1b9420c
Compare
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_memory_native/src/service.rs`:
- Around line 31-35: Stop re-exporting MemoryServiceMetadataResponse through
ironclaw_memory_native: in crates/ironclaw_memory_native/src/service.rs lines
31-35, make its import private for local implementation use, and in
crates/ironclaw_memory_native/src/lib.rs lines 61-65 remove the facade re-export
so consumers import the type from ironclaw_memory.
In `@crates/ironclaw_reborn_cli/src/commands/doctor.rs`:
- Around line 269-275: Remove marker.display() and path.display() from
user-facing error contexts in the doctor command, including the onboarding
marker read/parse handling around serde_json::from_str and the migration
manifest handling at the referenced later block. Return opaque messages such as
“failed to read onboarding marker,” “invalid onboarding marker,” or “invalid
migration manifest,” while retaining absolute paths only in debug! diagnostics.
In `@crates/ironclaw_reborn_cli/src/commands/onboard.rs`:
- Around line 78-87: Update the dry-run paths in the onboarding command,
including print_dry_run calls, to use the same marker-overwrite condition as the
real execution path. Ensure --dry-run --migrate-v1 reports rewriting an existing
marker when migration_requested and the applicable force logic would rewrite it,
while preserving the existing behavior otherwise.
In `@crates/ironclaw_reborn_identity/src/lib.rs`:
- Around line 121-125: Change the UserImportConflict payload from String to
UserId to preserve typed identity validation. Update all constructors and call
sites that create this error to pass user.user_id.clone() directly, and retain
the existing error message formatting.
In `@crates/ironclaw_reborn_migration/src/convert/extensions.rs`:
- Around line 39-62: Update record_tool_disposition and
record_channel_disposition, along with their call sites in the user iteration,
to accept the current user and include that user in each loss record’s
source_id. Ensure tool and channel identifiers remain uniquely attributable per
user while preserving the existing disposition details.
In `@crates/ironclaw_reborn_migration/src/convert/memory.rs`:
- Around line 37-47: Update the engine-document filtering around is_engine_path
and has_engine_converter so only paths matching the owning converters’ exact
engine-path classifiers are treated as converter-owned; do not suppress losses
for near matches such as nested mission.json or non-thread /threads/ paths.
Ensure every known v1 artifact receives an explicit migration or loss
disposition, and add negative tests covering these malformed or unsupported
near-match paths.
In `@crates/ironclaw_reborn_migration/src/convert/projects.rs`:
- Around line 71-77: Remove project.workspace_path from the legacy_engine_v2
metadata object in the project conversion logic, or replace it with a non-path
redacted migration marker. Ensure canonical ProjectRecord metadata never
persists or exposes the raw backend workspace path while preserving the other
legacy fields.
In `@crates/ironclaw_reborn_migration/src/convert/secrets.rs`:
- Around line 181-199: Replace the raw error interpolation in the metadata and
material_matches mappings within the secret migration flow with a fixed safe
operator-facing reason. Preserve the typed backend error as an internal
source/diagnostic cause, and ensure MigrationError exposed through the CLI never
includes the original error text.
In `@crates/ironclaw_reborn_migration/src/convert/threads.rs`:
- Around line 226-274: The idempotent replay validation in the User branch must
compare the complete persisted inbound-message identity, not only message ID,
kind, and content. Update the replay read/check around accept_inbound_message to
also validate actor_id and source_binding_id (preferably via a full-record
comparison or equivalent service-level enforcement), and reject divergent
deterministic-slot collisions without overwriting.
In `@crates/ironclaw_reborn_migration/src/convert/users.rs`:
- Around line 41-50: Decouple API-token reporting from the user loop in the
migration flow around report_api_tokens and the related ranges: enumerate token
owners or token rows directly so orphaned tokens and tokens remain discoverable
when the users table is absent, deduplicate processing by token ID, and assign
every known v1 persistent API-token category a re-authentication or reinstall
disposition.
In `@crates/ironclaw_reborn_migration/src/inventory.rs`:
- Around line 430-498: The home artifact enumeration in the read_dir(home)
branch is nondeterministic because entries are appended to out in filesystem
order. Collect the eligible directory entries before processing, sort them by a
stable path or filename key, then run the existing type, count, checksum, and
InventoryEntry construction logic in that sorted order so inventory_checksum
remains reproducible.
In `@crates/ironclaw_reborn_migration/src/source.rs`:
- Around line 288-313: Update the PostgreSQL branch of collect_inventory so it
does not sort or materialize all row JSON values into encoded_rows. Stream each
table’s rows from client, increment the count, and fold each encoded row into an
order-independent digest while processing; update or reuse
postgres_table_content_checksum as needed to produce the final checksum without
retaining every row.
In `@crates/ironclaw_reborn_migration/tests/migration_roundtrip.rs`:
- Around line 886-911: Update dry_run_reports_without_writing to call
plan_migration directly instead of run_migration, preserving the manifest and
dry-run assertions. Remove the now-unused legacy wrapper import and keep the
test on the explicit planning lifecycle.
In `@crates/ironclaw_secrets/src/lib.rs`:
- Around line 1023-1033: Implement material_matches in the shared secret-store
adapter path instead of returning StoreUnavailable, delegating the comparison to
the underlying store so ScopedSecretsStoreAdapter<S> and InMemorySecretStore can
participate in migration compare-and-apply. Trace the existing adapter
delegation methods to preserve scope and error behavior, then add a migration
test covering the full compare-and-apply caller chain in the secrets conversion
flow.
In `@docker/reborn/entrypoint.sh`:
- Around line 37-50: Update the migrate dispatch logic in
docker/reborn/entrypoint.sh so v1:verify exits directly through ironclaw-reborn
like other read-only operations, before persistent-volume checks or config
seeding. Remove it from the mutating/resume whitelist while preserving the
existing handling for v1:apply and v1:resume.
In `@src/db/libsql/mod.rs`:
- Around line 94-97: Update Database::new_local_read_only to open the source
database in immutable mode, using libsql’s immutable option or equivalent rather
than only SQLITE_OPEN_READ_ONLY. Ensure opening the database cannot create or
modify its -wal/-shm files or otherwise alter the source directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4d7a53ff-10dd-4914-83fa-b5b359dbac6c
⛔ Files ignored due to path filters (2)
CHANGELOG.mdis excluded by!CHANGELOG.mdCargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (74)
Dockerfile.rebornFEATURE_PARITY.mdREADME.mdcrates/AGENTS.mdcrates/README.mdcrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/CLAUDE.mdcrates/ironclaw_memory_native/src/backend.rscrates/ironclaw_memory_native/src/lib.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_reborn_cli/AGENTS.mdcrates/ironclaw_reborn_cli/Cargo.tomlcrates/ironclaw_reborn_cli/src/commands/doctor.rscrates/ironclaw_reborn_cli/src/commands/extension.rscrates/ironclaw_reborn_cli/src/commands/migrate.rscrates/ironclaw_reborn_cli/src/commands/mod.rscrates/ironclaw_reborn_cli/src/commands/onboard.rscrates/ironclaw_reborn_cli/src/context.rscrates/ironclaw_reborn_cli/tests/migration_cli.rscrates/ironclaw_reborn_cli/tests/smoke.rscrates/ironclaw_reborn_composition/AGENTS.mdcrates/ironclaw_reborn_composition/CLAUDE.mdcrates/ironclaw_reborn_composition/src/admin_user_directory.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/input.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/migration_support.rscrates/ironclaw_reborn_identity/CONTRACT.mdcrates/ironclaw_reborn_identity/src/filesystem_store/directory.rscrates/ironclaw_reborn_identity/src/filesystem_store/tests.rscrates/ironclaw_reborn_identity/src/lib.rscrates/ironclaw_reborn_identity/src/user_directory.rscrates/ironclaw_reborn_migration/CLAUDE.mdcrates/ironclaw_reborn_migration/Cargo.tomlcrates/ironclaw_reborn_migration/src/convert/automations.rscrates/ironclaw_reborn_migration/src/convert/extensions.rscrates/ironclaw_reborn_migration/src/convert/memory.rscrates/ironclaw_reborn_migration/src/convert/mod.rscrates/ironclaw_reborn_migration/src/convert/projects.rscrates/ironclaw_reborn_migration/src/convert/secrets.rscrates/ironclaw_reborn_migration/src/convert/threads.rscrates/ironclaw_reborn_migration/src/convert/users.rscrates/ironclaw_reborn_migration/src/error.rscrates/ironclaw_reborn_migration/src/inventory.rscrates/ironclaw_reborn_migration/src/lib.rscrates/ironclaw_reborn_migration/src/main.rscrates/ironclaw_reborn_migration/src/manifest.rscrates/ironclaw_reborn_migration/src/mounts.rscrates/ironclaw_reborn_migration/src/options.rscrates/ironclaw_reborn_migration/src/report.rscrates/ironclaw_reborn_migration/src/source.rscrates/ironclaw_reborn_migration/src/target.rscrates/ironclaw_reborn_migration/src/target_ids.rscrates/ironclaw_reborn_migration/src/v2_model.rscrates/ironclaw_reborn_migration/tests/companion_cli.rscrates/ironclaw_reborn_migration/tests/migration_roundtrip.rscrates/ironclaw_reborn_migration/tests/migration_safety.rscrates/ironclaw_reborn_migration/tests/project_migration.rscrates/ironclaw_secrets/src/filesystem_store.rscrates/ironclaw_secrets/src/lib.rsdocker/reborn/entrypoint.shdocs/internal/2026-06-26-legacy-vs-reborn-feature-comparison.mddocs/plans/8513-cli-smoke-and-secrets-decomposition.mddocs/plans/composition-pubuse.snapshotdocs/reborn-binary.mddocs/reborn/README.mddocs/reborn/contracts/memory.mddocs/reborn/contracts/migration-compatibility.mddocs/reborn/deploy-reborn-cli-docker.mddocs/reborn/onboarding.mddocs/reborn/v1-migration.mdsrc/db/libsql/mod.rssrc/setup/README.md
| MemoryServiceErrorKind, MemoryServiceMetadataResponse, MemoryServiceProfileSetRequest, | ||
| MemoryServiceProfileSetResponse, MemoryServiceReadRequest, MemoryServiceReadResponse, | ||
| MemoryServiceSearchRequest, MemoryServiceSearchResponse, MemoryServiceSearchResult, | ||
| MemoryServiceTreeRequest, MemoryServiceTreeResponse, MemoryServiceWriteRequest, | ||
| MemoryServiceWriteResponse, MemoryWriteStatus, memory_context_disabled, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Import MemoryServiceMetadataResponse from its owning crate.
The new two-stage re-export repackages an ironclaw_memory contract type through ironclaw_memory_native.
crates/ironclaw_memory_native/src/service.rs#L31-L35: make this a privateusefor the implementation instead ofpub use.crates/ironclaw_memory_native/src/lib.rs#L61-L65: remove the facade re-export; downstream consumers should import it fromironclaw_memory.
As per coding guidelines, “consumers should import the owner’s type” and pub use must not be a path-preservation shim.
📍 Affects 2 files
crates/ironclaw_memory_native/src/service.rs#L31-L35(this comment)crates/ironclaw_memory_native/src/lib.rs#L61-L65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_memory_native/src/service.rs` around lines 31 - 35, Stop
re-exporting MemoryServiceMetadataResponse through ironclaw_memory_native: in
crates/ironclaw_memory_native/src/service.rs lines 31-35, make its import
private for local implementation use, and in
crates/ironclaw_memory_native/src/lib.rs lines 61-65 remove the facade re-export
so consumers import the type from ironclaw_memory.
Source: Coding guidelines
| return Err(error).with_context(|| { | ||
| format!("failed to read onboarding marker at {}", marker.display()) | ||
| }); | ||
| } | ||
| }; | ||
| let document: serde_json::Value = serde_json::from_str(&body) | ||
| .with_context(|| format!("invalid onboarding marker at {}", marker.display()))?; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not expose absolute migration paths in doctor output.
These contexts include marker.display() and path.display(); Line 236 then embeds the error in a user-facing check. Keep paths in debug! diagnostics and return an opaque message such as invalid onboarding marker or invalid migration manifest.
As per coding guidelines, “at channel boundaries, do not let internal identifiers, tracebacks, transport errors, absolute paths, internal file names, or wire-format prefixes reach user-facing output.”
Also applies to: 303-306
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_cli/src/commands/doctor.rs` around lines 269 - 275,
Remove marker.display() and path.display() from user-facing error contexts in
the doctor command, including the onboarding marker read/parse handling around
serde_json::from_str and the migration manifest handling at the referenced later
block. Return opaque messages such as “failed to read onboarding marker,”
“invalid onboarding marker,” or “invalid migration manifest,” while retaining
absolute paths only in debug! diagnostics.
Source: Coding guidelines
| if self.dry_run { | ||
| print_dry_run(home, &marker_path, self.force, self.import_history); | ||
| print_dry_run( | ||
| home, | ||
| &marker_path, | ||
| self.force, | ||
| migration_requested, | ||
| self.skip_v1_migration, | ||
| source.as_ref(), | ||
| &manifest_path, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make dry-run use the actual marker overwrite condition.
With an existing marker, --dry-run --migrate-v1 reports preservation, but the real command forces a rewrite.
Proposed fix
+ let force_marker =
+ self.force || migration_requested || self.skip_v1_migration;
+
if self.dry_run {
print_dry_run(
home,
&marker_path,
- self.force,
+ force_marker,
migration_requested,
self.skip_v1_migration,
source.as_ref(),
&manifest_path,
);
@@
- self.force || migration_requested || self.skip_v1_migration,
+ force_marker,Also applies to: 110-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_cli/src/commands/onboard.rs` around lines 78 - 87,
Update the dry-run paths in the onboarding command, including print_dry_run
calls, to use the same marker-overwrite condition as the real execution path.
Ensure --dry-run --migrate-v1 reports rewriting an existing marker when
migration_requested and the applicable force logic would rewrite it, while
preserving the existing behavior otherwise.
| /// A historical import targeted a user id that already contains different | ||
| /// canonical state. Migration must stop rather than overwrite live or | ||
| /// previously imported user data. | ||
| #[error("migrated user conflicts with existing record for id: {0}")] | ||
| UserImportConflict(String), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep the conflicting identity typed as UserId.
Using String weakens this public contract and permits invalid identifiers.
Proposed fix
- UserImportConflict(String),
+ UserImportConflict(UserId),Update constructors to pass user.user_id.clone() directly.
As per coding guidelines, “Identifiers must use newtypes such as CredentialName, ExtensionName, ThreadId, or UserId.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// A historical import targeted a user id that already contains different | |
| /// canonical state. Migration must stop rather than overwrite live or | |
| /// previously imported user data. | |
| #[error("migrated user conflicts with existing record for id: {0}")] | |
| UserImportConflict(String), | |
| /// A historical import targeted a user id that already contains different | |
| /// canonical state. Migration must stop rather than overwrite live or | |
| /// previously imported user data. | |
| #[error("migrated user conflicts with existing record for id: {0}")] | |
| UserImportConflict(UserId), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_identity/src/lib.rs` around lines 121 - 125, Change
the UserImportConflict payload from String to UserId to preserve typed identity
validation. Update all constructors and call sites that create this error to
pass user.user_id.clone() directly, and retain the existing error message
formatting.
Source: Coding guidelines
| for user in users { | ||
| if let Some(store) = tool_store.as_ref() { | ||
| let tools = store | ||
| for tool in store | ||
| .list(&user) | ||
| .await | ||
| .map_err(|e| MigrationError::ReadSource { | ||
| .map_err(|error| MigrationError::ReadSource { | ||
| domain: "wasm_tools".into(), | ||
| reason: e.to_string(), | ||
| })?; | ||
| for tool in tools { | ||
| let bindings = tool_credential_bindings(store.as_ref(), &tool, report).await?; | ||
| collect_installation( | ||
| report, | ||
| &mut candidates_by_extension, | ||
| InstallInput { | ||
| owner: &user, | ||
| raw_name: &tool.name, | ||
| version: &tool.version, | ||
| description: &tool.description, | ||
| active: tool.status == ToolStatus::Active, | ||
| updated_at: tool.updated_at, | ||
| bindings, | ||
| }, | ||
| ); | ||
| reason: error.to_string(), | ||
| })? | ||
| { | ||
| record_tool_disposition(store.as_ref(), &tool, report).await?; | ||
| } | ||
| } | ||
|
|
||
| if let Some(store) = channel_store.as_ref() { | ||
| let channels = store | ||
| for channel in store | ||
| .list(&user) | ||
| .await | ||
| .map_err(|e| MigrationError::ReadSource { | ||
| .map_err(|error| MigrationError::ReadSource { | ||
| domain: "wasm_channels".into(), | ||
| reason: e.to_string(), | ||
| })?; | ||
| for channel in channels { | ||
| collect_installation( | ||
| report, | ||
| &mut candidates_by_extension, | ||
| channel_input(&user, &channel), | ||
| ); | ||
| report.record_loss( | ||
| Domain::Extension, | ||
| format!("channel:{}", channel.name), | ||
| "credential_binding", | ||
| LossReason::NoTargetField, | ||
| "v1 has no explicit channel→secret join; the credential value still \ | ||
| migrates via the secrets converter, but the installation binding is \ | ||
| not auto-linked" | ||
| .to_string(), | ||
| ); | ||
| reason: error.to_string(), | ||
| })? | ||
| { | ||
| record_channel_disposition(&channel, report); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include user scope in extension loss identifiers.
tool:{name} and channel:{name} collide across users, making per-record losses ambiguous. Pass user into both helpers and include it in source_id.
Proposed fix
- record_tool_disposition(store.as_ref(), &tool, report).await?;
+ record_tool_disposition(store.as_ref(), &user, &tool, report).await?;
- record_channel_disposition(&channel, report);
+ record_channel_disposition(&user, &channel, report);
async fn record_tool_disposition(
store: &dyn WasmToolStore,
+ user: &str,
tool: &StoredWasmTool,
report: &mut MigrationReport,
) -> Result<(), MigrationError> {
- let source_id = format!("tool:{}", tool.name);
+ let source_id = format!("user:{user}:tool:{}", tool.name);
-fn record_channel_disposition(channel: &StoredWasmChannel, report: &mut MigrationReport) {
- let source_id = format!("channel:{}", channel.name);
+fn record_channel_disposition(
+ user: &str,
+ channel: &StoredWasmChannel,
+ report: &mut MigrationReport,
+) {
+ let source_id = format!("user:{user}:channel:{}", channel.name);As per coding guidelines, migration records must preserve user scope and converter loss details must remain attributable to the source record.
Also applies to: 69-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_migration/src/convert/extensions.rs` around lines 39 -
62, Update record_tool_disposition and record_channel_disposition, along with
their call sites in the user iteration, to accept the current user and include
that user in each loss record’s source_id. Ensure tool and channel identifiers
remain uniquely attributable per user while preserving the existing disposition
details.
Source: Coding guidelines
| for row in rows { | ||
| let name: String = row | ||
| .try_get(0) | ||
| .map_err(|error| source_read_error("inventory", error))?; | ||
| let sql = format!( | ||
| "SELECT to_jsonb(source_row)::text FROM {} AS source_row \ | ||
| ORDER BY to_jsonb(source_row)::text", | ||
| quote_identifier(&name), | ||
| ); | ||
| let rows = client | ||
| .query(&sql, &[]) | ||
| .await | ||
| .map_err(|error| source_read_error(&name, error))?; | ||
| let encoded_rows = rows | ||
| .iter() | ||
| .map(|row| { | ||
| row.try_get::<_, String>(0) | ||
| .map_err(|error| source_read_error(&name, error)) | ||
| }) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
| inventory.push(RawTableInventory { | ||
| name: name.clone(), | ||
| count: encoded_rows.len() as u64, | ||
| checksum: postgres_table_content_checksum(&name, &encoded_rows), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm table_inventory is on the planning hot path and gauge v1 table cardinality expectations.
rg -nP '\btable_inventory\s*\(' crates/ironclaw_reborn_migration/src -C2
rg -nP 'to_jsonb\(source_row\)' crates/ironclaw_reborn_migration/src/source.rs -C2Repository: nearai/ironclaw
Length of output: 1996
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the table_inventory implementation and the fingerprint/plan call path.
sed -n '120,340p' crates/ironclaw_reborn_migration/src/source.rs
printf '\n--- lib.rs call site ---\n'
sed -n '450,510p' crates/ironclaw_reborn_migration/src/lib.rs
# Check for any comments/docs indicating expected table sizes or streaming constraints.
rg -n "messages|audit|inventory|fingerprint|table_inventory|stream" crates/ironclaw_reborn_migration -g '!target'Repository: nearai/ironclaw
Length of output: 32374
PG table_inventory should not materialize every row during planning.
collect_inventory() calls this on the plan/fingerprint path, and the PG branch reads to_jsonb(source_row)::text for every row of every table, sorts by the full JSON text, then collects the whole result set into Vec<String>. On large messages/audit tables this is an OOM/latency risk; stream rows and fold an order-independent digest instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_migration/src/source.rs` around lines 288 - 313,
Update the PostgreSQL branch of collect_inventory so it does not sort or
materialize all row JSON values into encoded_rows. Stream each table’s rows from
client, increment the count, and fold each encoded row into an order-independent
digest while processing; update or reuse postgres_table_content_checksum as
needed to produce the final checksum without retaining every row.
| #[tokio::test] | ||
| async fn dry_run_reports_without_writing() { | ||
| let dir = tempfile::tempdir().unwrap(); | ||
| let src = seed_v1_fixture(dir.path()).await; | ||
| let dst = dir.path().join("reborn-dry.db"); | ||
| let target_parent = dir.path().join("missing-target"); | ||
| let dst = target_parent.join("reborn-dry.db"); | ||
|
|
||
| let report = run_migration(options(src, dst.clone(), true)) | ||
| .await | ||
| .expect("dry run"); | ||
|
|
||
| // Same counts as a real run … | ||
| assert_eq!(report.stats.threads, 3); | ||
| assert_eq!(report.stats.routines, 2); | ||
| assert_eq!(report.stats.missions, 2); | ||
| // Planning reports inventory through the manifest without invoking writers. | ||
| let manifest = report.manifest.expect("plan manifest"); | ||
| assert!( | ||
| manifest | ||
| .inventory | ||
| .iter() | ||
| .any(|item| item.source_name == "conversations" && item.count >= 2) | ||
| ); | ||
| assert!(report.dry_run); | ||
|
|
||
| // … but nothing was written to the Reborn store. | ||
| // No read helper is invoked here: opening libSQL would itself create state. | ||
| assert!( | ||
| reborn_triggers(&dst).await.is_empty(), | ||
| "dry run wrote triggers" | ||
| ); | ||
| assert_eq!( | ||
| reborn_thread_doc_count(&dst).await, | ||
| 0, | ||
| "dry run wrote thread docs" | ||
| !dst.exists() && !target_parent.exists(), | ||
| "planning created the target path or its parent" | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm run_migration is deprecated and whether a plan-only path suffices for the dry-run assertions.
rg -nP '\brun_migration\b' crates/ironclaw_reborn_migration/src
ast-grep run --pattern 'pub async fn run_migration($$$) { $$$ }' --lang rust crates/ironclaw_reborn_migration/srcRepository: nearai/ironclaw
Length of output: 809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect migration entrypoints and the dry-run test context.
ast-grep outline crates/ironclaw_reborn_migration/src/lib.rs --view expanded
printf '\n--- lib.rs around run_migration/plan_migration ---\n'
sed -n '220,360p' crates/ironclaw_reborn_migration/src/lib.rs
printf '\n--- test file around dry_run_reports_without_writing ---\n'
sed -n '1,140p' crates/ironclaw_reborn_migration/tests/migration_roundtrip.rs
sed -n '860,930p' crates/ironclaw_reborn_migration/tests/migration_roundtrip.rsRepository: nearai/ironclaw
Length of output: 14957
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '51,131p' crates/ironclaw_reborn_migration/src/lib.rsRepository: nearai/ironclaw
Length of output: 3529
Use plan_migration here instead of run_migration. This test only needs the source-read-only planning path, which already returns the manifest without opening the target; drop the legacy wrapper and its import to stay on the explicit plan|apply|resume|verify lifecycle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_migration/tests/migration_roundtrip.rs` around lines
886 - 911, Update dry_run_reports_without_writing to call plan_migration
directly instead of run_migration, preserving the manifest and dry-run
assertions. Remove the now-unused legacy wrapper import and keep the test on the
explicit planning lifecycle.
Source: Coding guidelines
| # Migration is always an explicit operator workflow. Read-only planning, | ||
| # status, and help must not let the container entrypoint create target state or | ||
| # seed config first. Mutating/resume/verify operations continue through the | ||
| # normal persistent-volume and config checks below. | ||
| if [ "${1:-}" = "migrate" ]; then | ||
| for migration_arg in "$@"; do | ||
| case "$migration_arg" in | ||
| -h|--help) exec ironclaw-reborn "$@" ;; | ||
| esac | ||
| done | ||
| case "${2:-}:${3:-}" in | ||
| v1:apply|v1:resume|v1:verify) ;; | ||
| *) exec ironclaw-reborn "$@" ;; | ||
| esac |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep verify read-only at the container entrypoint.
Whitelisting v1:verify runs normal initialization and config seeding before verification. An empty target can therefore be mutated before it is inspected.
Proposed fix
- v1:apply|v1:resume|v1:verify) ;;
+ v1:apply|v1:resume) ;;As per coding guidelines, “verify must perform read-only checks.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Migration is always an explicit operator workflow. Read-only planning, | |
| # status, and help must not let the container entrypoint create target state or | |
| # seed config first. Mutating/resume/verify operations continue through the | |
| # normal persistent-volume and config checks below. | |
| if [ "${1:-}" = "migrate" ]; then | |
| for migration_arg in "$@"; do | |
| case "$migration_arg" in | |
| -h|--help) exec ironclaw-reborn "$@" ;; | |
| esac | |
| done | |
| case "${2:-}:${3:-}" in | |
| v1:apply|v1:resume|v1:verify) ;; | |
| *) exec ironclaw-reborn "$@" ;; | |
| esac | |
| # Migration is always an explicit operator workflow. Read-only planning, | |
| # status, and help must not let the container entrypoint create target state or | |
| # seed config first. Mutating/resume/verify operations continue through the | |
| # normal persistent-volume and config checks below. | |
| if [ "${1:-}" = "migrate" ]; then | |
| for migration_arg in "$@"; do | |
| case "$migration_arg" in | |
| -h|--help) exec ironclaw-reborn "$@" ;; | |
| esac | |
| done | |
| case "${2:-}:${3:-}" in | |
| v1:apply|v1:resume) ;; | |
| *) exec ironclaw-reborn "$@" ;; | |
| esac |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker/reborn/entrypoint.sh` around lines 37 - 50, Update the migrate
dispatch logic in docker/reborn/entrypoint.sh so v1:verify exits directly
through ironclaw-reborn like other read-only operations, before
persistent-volume checks or config seeding. Remove it from the mutating/resume
whitelist while preserving the existing handling for v1:apply and v1:resume.
Source: Coding guidelines
| pub async fn new_local_read_only(path: &Path) -> Result<Self, DatabaseError> { | ||
| let db = libsql::Builder::new_local(path) | ||
| .flags(libsql::OpenFlags::SQLITE_OPEN_READ_ONLY) | ||
| .build() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
For SQLite WAL databases and libsql 0.9, can opening a database with SQLITE_OPEN_READ_ONLY create or modify -shm or -wal files, and which immutable URI flag or libsql API prevents all source-directory writes?
💡 Result:
When opening a SQLite database (including those using WAL mode) with the SQLITE_OPEN_READ_ONLY flag, the database connection is prohibited from performing write operations [1][2]. However, because WAL mode relies on the creation and modification of auxiliary files—specifically the write-ahead log (-wal) and shared-memory (-shm) files—opening a WAL database in read-only mode can fail if the process does not have the necessary permissions to create or update these files in the source directory [3][4][5]. Even in read-only mode, SQLite's VFS (Virtual File System) layer may attempt to open these auxiliary files to ensure consistent access, which effectively requires write access to the filesystem if those files do not already exist or need to be recovered [3][4]. To prevent all writes to the source directory when opening a database, the immutable URI parameter is used [1]. By appending?immutable=1 to the database connection string, you instruct SQLite (and by extension, libsql) to treat the database file as completely read-only and static [1]. This flag disables all attempts to create or modify auxiliary files, including the journal or WAL files, as it guarantees that no other process will modify the database and that the database is stored in a location that cannot be changed [1]. This is the standard mechanism to ensure the source directory remains untouched [1].
Citations:
- 1: https://sqlite.org/c3ref/open.html
- 2: https://docs.rs/libsql-rusqlite/latest/rusqlite/struct.OpenFlags.html
- 3: https://sqlite.org/wal.html
- 4: https://stackoverflow.com/questions/12187023/write-ahead-logging-and-read-only-mode-compatible-in-sqlite3
- 5: https://www.sqlite.org/isolation.html
🏁 Script executed:
sed -n '1,180p' src/db/libsql/mod.rsRepository: nearai/ironclaw
Length of output: 6577
🏁 Script executed:
sed -n '520,590p' src/db/libsql/mod.rsRepository: nearai/ironclaw
Length of output: 2996
🏁 Script executed:
rg -n "new_local_read_only|immutable=1|SQLITE_OPEN_READ_ONLY|new_local\(" src/db/libsql src/db -g '*.rs'Repository: nearai/ironclaw
Length of output: 5999
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('src/db/libsql/mod.rs')
for start, end in [(1,180),(520,590)]:
print(f"\n--- {start}-{end} ---")
with p.open() as f:
for i, line in enumerate(f, 1):
if start <= i <= end:
print(f"{i:4}: {line.rstrip()}")
PYRepository: nearai/ironclaw
Length of output: 10958
🏁 Script executed:
sed -n '1,180p' src/db/libsql/mod.rs && printf '\n====\n' && sed -n '520,590p' src/db/libsql/mod.rsRepository: nearai/ironclaw
Length of output: 9426
Use immutable mode for the read-only source path
SQLITE_OPEN_READ_ONLY only blocks writes to source.db; WAL access can still create or touch -wal/-shm. The source-immutability rule for migration tooling isn’t covered by a single-file snapshot, so this path should use immutable open mode (immutable=1 or equivalent) or assert the entire source directory stays unchanged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db/libsql/mod.rs` around lines 94 - 97, Update
Database::new_local_read_only to open the source database in immutable mode,
using libsql’s immutable option or equivalent rather than only
SQLITE_OPEN_READ_ONLY. Ensure opening the database cannot create or modify its
-wal/-shm files or otherwise alter the source directory.
Source: Coding guidelines
1b9420c to
9e41fe2
Compare
serrrfirat
changed the title
There was a problem hiding this comment.
Actionable comments posted: 23
♻️ Duplicate comments (4)
docker/reborn/entrypoint.sh (1)
41-51: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
v1:verifystill routed through mutating init/config-seeding before running.
v1:verifyremains whitelisted next tov1:apply/v1:resume, so it falls through to the normal persistent-volume and config-file installation logic below this guard rather than exiting immediately likeplan/status/--help. This lets the entrypoint create/seed target state before a nominally read-onlyverifyeven runs — same issue raised in a prior review, still present.🐛 Proposed fix
- v1:apply|v1:resume|v1:verify) ;; + v1:apply|v1:resume) ;;As per coding guidelines, "
verifymust perform read-only target-data checks... it may write lifecycle/quarantine state but must not boot a full Reborn runtime."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker/reborn/entrypoint.sh` around lines 41 - 51, Update the migrate command routing in the entrypoint guard so v1:verify exits through the read-only migration command path instead of remaining in the mutating whitelist with v1:apply and v1:resume. Preserve the existing handling for apply/resume while ensuring verify runs target-data checks without persistent-volume or config-file initialization and without booting the full Reborn runtime.Source: Coding guidelines
crates/ironclaw_reborn_cli/src/commands/onboard.rs (1)
78-89: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDry-run still reports marker preservation when the real run would force a rewrite.
print_dry_runat Line 82 receives plainself.force, but the actual marker write at Line 113 usesself.force || migration_requested || self.skip_v1_migration.--dry-run --migrate-v1(or--skip-v1-migration) against an existing marker printswould_preservewhile the real invocation with the same flags rewrites the marker. This is the same mismatch raised in a prior review and does not appear to have been fixed here.🐛 Proposed fix
+ let force_marker = self.force || migration_requested || self.skip_v1_migration; + if self.dry_run { print_dry_run( home, &marker_path, - self.force, + force_marker, migration_requested, self.skip_v1_migration, source.as_ref(), &manifest_path, ); return Ok(()); } @@ let marker_action = write_onboarding_marker( home, &marker_path, - self.force || migration_requested || self.skip_v1_migration, + force_marker, migration_state, &manifest_path, )?;Also applies to: 110-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_cli/src/commands/onboard.rs` around lines 78 - 89, Update the dry-run call in the onboarding flow around print_dry_run to pass the same effective force condition used by the real marker write: self.force || migration_requested || self.skip_v1_migration. Keep marker reporting consistent between dry-run and execution, including when migration or skip-migration flags are set.crates/ironclaw_reborn_migration/src/convert/projects.rs (1)
71-78: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winStill persisting the legacy host
workspace_pathinto project metadata — unresolved from last review.
project.workspace_pathis a raw v1 host filesystem path, copied verbatim into canonicalProjectRecord.metadatawhere it becomes readable through the project surface. Per the coding guideline "do not expose raw... backend paths... across public surfaces," this should be dropped or redacted to a marker.🔒 Proposed fix
metadata: json!({ "legacy_engine_v2": { "goals": project.goals, "metrics": project.metrics, "metadata": project.metadata, - "workspace_path": project.workspace_path, + "workspace_migrated": project.workspace_path.is_some(), } }),As per coding guidelines, "Do not expose raw secrets, backend paths, private URLs, transport internals, raw SQL/backend errors, or unredacted runtime or user content across public surfaces."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_migration/src/convert/projects.rs` around lines 71 - 78, Remove the workspace_path field from the legacy_engine_v2 object constructed in the project conversion metadata, or replace its raw value with the project’s established redaction marker. Keep the goals, metrics, and metadata fields unchanged, and ensure no raw v1 host filesystem path reaches ProjectRecord.metadata.Source: Coding guidelines
crates/ironclaw_reborn_migration/src/convert/memory.rs (1)
37-49: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
has_engine_converterstill uses the same loose grammar the last review flagged — and no mission converter exists in this diff to back it up.
ends_with("mission.json")andcontains("/threads/") && ends_with(".json")match any nested path, not just the owning converter's exact grammar (mirrorsprojects.rs'sproject_slug, which does use exact segment matching). Worse:convert/mod.rsregistersautomations,extensions,heartbeat,memory,projects,secrets,settings,threads,users— there is nomissionsconverter module, andthreads.rsis presumably the v1threadstable converter, not an engine-v2/threads/*.jsonblob reader. If nothing actually consumes these paths,has_engine_converterreturningtruecauses line 48 to silently drop the document with norecord_loss— the exact scenario "every known v1 artifact must receive an explicit disposition" was written to prevent. The tests only cover exact-path positives/negatives, not nested near-matches (engine/runtime/x/mission.json,foo/threads/bar.json).♻️ Proposed fix: exact segment grammar mirroring project_slug
fn has_engine_converter(path: &str) -> bool { - if path.ends_with("mission.json") || (path.contains("/threads/") && path.ends_with(".json")) { - return true; - } let segments: Vec<_> = path.trim_matches('/').split('/').collect(); matches!( segments.as_slice(), ["engine", "projects", slug, "project.json"] | [".system", "engine", "projects", slug, "project.json"] + | ["engine", "missions", _, "mission.json"] + | [".system", "engine", "missions", _, "mission.json"] + | ["engine", "threads", _] + | [".system", "engine", "threads", _] if !slug.is_empty() ) }Run this to confirm no mission/thread converter actually reads
memory_documentsat engine-v2 paths:#!/bin/bash fd . crates/ironclaw_reborn_migration/src/convert -e rs rg -n 'mission' crates/ironclaw_reborn_migration/src/convert/automations.rs crates/ironclaw_reborn_migration/src/convert/threads.rs 2>/dev/null rg -n 'memory_documents|all_memory_documents' crates/ironclaw_reborn_migration/src -g '*.rs'Also applies to: 175-186, 188-200
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_migration/src/convert/memory.rs` around lines 37 - 49, The engine-path converter detection used by has_engine_converter must only recognize paths with an actually supported exact grammar. Remove mission and engine-v2 thread matches unless corresponding converters consume those documents, and tighten any retained matching to exact path segments rather than broad suffix or substring checks, mirroring projects.rs project_slug behavior. Ensure unsupported nested near-matches return false so the caller records LossReason::NoTargetConcept instead of silently dropping documents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_cli/src/commands/extension.rs`:
- Around line 135-155: Update the quarantine test around
ExtensionCommand::execute to create valid matching local and durable migration
state with status “applying,” including all required binding fields. Invoke the
command through ExtensionCommand::execute and assert the specific
applying-status lifecycle error, ensuring the test exercises policy gating
rather than marker deserialization failure.
In `@crates/ironclaw_reborn_cli/src/commands/migrate.rs`:
- Around line 31-42: Update MigrationStateRecord to include release_version and
require it to equal env!("CARGO_PKG_VERSION") when reading and validating
migration state from both local and durable backends. Ensure equality covers
release/protocol, profile, backend, locator fingerprint, tenant, and agent, and
add a regression test confirming matching current-release state is accepted
while older-release state is rejected.
- Around line 398-402: Update the target-resolution match in the migration
command to propagate resolver failures instead of converting Err(_) to Ok(None).
Add contextual error information while preserving the existing successful target
path and None behavior only for cases where resolution legitimately returns no
target.
In `@crates/ironclaw_reborn_composition/src/migration_support.rs`:
- Around line 195-211: Update the locator-fingerprint resolution around ancestor
canonicalization to propagate failures from canonicalize instead of using
unwrap_or(ancestor). Convert the error into RebornBuildError::InvalidConfig
through the enclosing function’s existing error path, while preserving the
missing-suffix reconstruction and hashing behavior after successful
canonicalization.
In `@crates/ironclaw_reborn_migration/src/convert/secrets.rs`:
- Around line 175-181: The put_if_absent_or_matches error mapping must not
expose raw SecretStoreError text through MigrationError::WriteTarget.reason. In
the surrounding migration function, replace e.to_string() with the same fixed,
non-sensitive reason pattern already used by secret_collision, while preserving
the existing domain and error propagation.
In `@crates/ironclaw_reborn_migration/src/lib.rs`:
- Around line 299-313: Retire the unsafe one-shot apply path in run_migration:
keep this wrapper limited to planning/dry-run, or require explicit lifecycle
safety acknowledgements before calling apply_migration instead of fabricating
ApplyAcknowledgements::offline_snapshot(). In
crates/ironclaw_reborn_migration/src/options.rs lines 50-60, remove target-key
derivation from the source key and require independent source and target keys.
- Around line 459-465: Enforce exact release compatibility at both public
lifecycle boundaries: update the manifest validation in
crates/ironclaw_reborn_migration/src/lib.rs (lines 459-465) to reject differing
release information, and add the same compatibility check in
crates/ironclaw_reborn_migration/src/main.rs (lines 575-587) before persisting
any CLI lifecycle transition. Use the existing manifest and version symbols,
preserving the current invalid-input error behavior.
- Around line 207-231: Centralize post-Applying failure handling in the
migration library: wrap V1Source::open, RebornTarget::open, converter execution,
and related post-claim errors so each produces, seals, persists, and returns one
authoritative Failed manifest. In crates/ironclaw_reborn_migration/src/lib.rs
lines 207-231, return the failed manifest with the error; in
crates/ironclaw_reborn_migration/src/main.rs lines 290-300, persist that
returned manifest instead of deriving another; and in lines 333-339 and 364-371,
reuse the authoritative resume and verification failure states without creating
competing lifecycle records.
- Around line 87-90: Update the target emptiness checks using target_empty in
both plan and apply flows to propagate filesystem metadata errors instead of
treating path.exists() failures as empty. Change canonicalish() to return and
propagate current_dir() and canonicalize() errors rather than falling back to
"." or the last reachable ancestor, preserving distinct-store and emptiness
validation for successful probes.
In `@crates/ironclaw_reborn_migration/src/manifest.rs`:
- Around line 154-168: Preserve specialized identity and profile types
throughout the resolved migration scope instead of converting them to strings.
In crates/ironclaw_reborn_migration/src/manifest.rs:154-168, update
ResolvedScope fields to use the owning domain types; in
crates/ironclaw_reborn_migration/src/options.rs:21-26, replace the raw profile
string with the profile type; in
crates/ironclaw_reborn_migration/src/main.rs:183-190, retain that resolved
profile type; and in crates/ironclaw_reborn_migration/src/main.rs:444-450,
remove the conversion to an owned string.
- Around line 267-316: Convert manifest persistence and all migration lifecycle
filesystem I/O to Tokio async: make Manifest::write_atomic async using async
filesystem primitives, update target-state persistence in
crates/ironclaw_reborn_migration/src/main.rs:454-533 to await it, and make
manifest reads and report persistence in
crates/ironclaw_reborn_migration/src/main.rs:575-599 async with awaited Tokio
operations; preserve the existing atomic, overwrite, permissions, and cleanup
behavior.
In `@crates/ironclaw_reborn_migration/src/mounts.rs`:
- Around line 19-34: Replace migration_mount_view_is_the_production_mount_view
with an integration test that invokes a real migration repository writer using
the test scope, then reopens the written record through
ironclaw_reborn_composition::invocation_mount_view. Assert the persisted record
is accessible through the production resolver, covering both resolver usage and
correct scope construction; avoid directly comparing the two mount-view
functions.
In `@crates/ironclaw_reborn_migration/src/source.rs`:
- Around line 799-807: Centralize sanitization for migration backend errors:
update source_open_error and source_read_error to emit the raw diagnostic via
debug! while returning stable redacted MigrationError messages; in
crates/ironclaw_reborn_migration/src/target.rs lines 1102-1107, remove {error}
from the public message and route direct target-driver mappings through the
existing sanitized helper. Ensure no raw backend error crosses a public surface.
- Around line 67-74: Update the metadata check in the async SourceDb::LibSql
branch of open to use Tokio’s asynchronous filesystem metadata API instead of
std::fs::metadata, while preserving the existing MigrationError::OpenSource
message and error propagation.
In `@crates/ironclaw_reborn_migration/src/target_ids.rs`:
- Around line 36-53: Introduce a private MigrationIdDomain enum for the
supported deterministic ID domains, with centralized stable seed labels for
routine, mission, and thread-binding. Update trigger_id and the related methods
around lines 75–97 to accept/use this enum and derive the scoped UUID seed from
its canonical label, eliminating direct stringly typed domain values while
preserving existing ID stability.
- Around line 23-33: Update MigrationIdentity::from_report to call
manifest.validate_plan_hash()? after confirming the manifest exists and before
copying manifest_schema_version or source_fingerprint. Propagate validation
failures as MigrationError and only derive target IDs from a validated sealed
manifest.
In `@crates/ironclaw_reborn_migration/src/target.rs`:
- Around line 1075-1098: Update the advisory-lock cleanup in the target lock
probe so both target and source unlock operations are attempted and their
results are propagated with contextual errors instead of discarded. Preserve the
original query error while reporting cleanup failures appropriately, and ensure
no path returns while either identity lock may remain held.
- Around line 46-50: Update target_is_empty for TargetStore::LibSql to open an
existing database read-only and inspect its durable tables for actual rows,
returning empty for pre-created empty or schema-only files while preserving
PostgreSQL parity. Keep the missing-path behavior unchanged, and add a
regression test covering an existing empty LibSQL file.
In `@crates/ironclaw_reborn_migration/tests/companion_cli.rs`:
- Around line 36-54: Expand
lifecycle_help_never_accepts_raw_postgres_urls_or_keys to invoke help for every
relevant v1 lifecycle subcommand, including plan, apply, and resume. Assert each
command exposes the supported source flags and rejects --source-postgres-url,
--target-postgres, and --secret-master-key, preserving the existing success and
output diagnostics.
In `@crates/ironclaw_reborn_migration/tests/migration_roundtrip.rs`:
- Around line 952-982: The test currently expects the stale manifest to fail
during the lifecycle transition before trigger collision reconciliation runs.
Add a separate resume scenario using an allowed migration state, invoke
resume_migration through the normal caller, and assert the divergent target-slot
collision error. Keep the existing assertion that the “cron-light” prompt
remains “divergent target prompt,” verifying the collision fails without
overwriting.
In `@crates/ironclaw_reborn_migration/tests/migration_safety.rs`:
- Around line 162-185: Update
plan_is_source_read_only_and_does_not_create_target to snapshot every file under
the source store directory, including relative paths and bytes, before
plan_migration and compare the complete snapshot afterward. Avoid
source_contents() or any reopening of the source during verification, and retain
the assertions that planning does not create the target database or parent
directory.
In `@crates/ironclaw_secrets/src/lib.rs`:
- Around line 1021-1037: Implement put_if_absent_or_matches for both
ScopedSecretsStoreAdapter<S> and InMemorySecretStore instead of inheriting the
StoreUnavailable default. Delegate through ScopedSecretsStoreAdapter to its
wrapped store with the appropriate scope handling, and add the equivalent atomic
insert, expiry, and material-match behavior to InMemorySecretStore so callers
preserve SecretPutOutcome semantics.
In `@docs/reborn-binary.md`:
- Around line 395-402: Clarify the onboarding documentation’s statement about v1
import state so it applies only to normal onboarding. Explicitly exempt the
read-only --migrate-v1 migration planning path, which reads detected v1 state
without applying changes, while preserving the existing behavior descriptions
for --dry-run and --import-history.
---
Duplicate comments:
In `@crates/ironclaw_reborn_cli/src/commands/onboard.rs`:
- Around line 78-89: Update the dry-run call in the onboarding flow around
print_dry_run to pass the same effective force condition used by the real marker
write: self.force || migration_requested || self.skip_v1_migration. Keep marker
reporting consistent between dry-run and execution, including when migration or
skip-migration flags are set.
In `@crates/ironclaw_reborn_migration/src/convert/memory.rs`:
- Around line 37-49: The engine-path converter detection used by
has_engine_converter must only recognize paths with an actually supported exact
grammar. Remove mission and engine-v2 thread matches unless corresponding
converters consume those documents, and tighten any retained matching to exact
path segments rather than broad suffix or substring checks, mirroring
projects.rs project_slug behavior. Ensure unsupported nested near-matches return
false so the caller records LossReason::NoTargetConcept instead of silently
dropping documents.
In `@crates/ironclaw_reborn_migration/src/convert/projects.rs`:
- Around line 71-78: Remove the workspace_path field from the legacy_engine_v2
object constructed in the project conversion metadata, or replace its raw value
with the project’s established redaction marker. Keep the goals, metrics, and
metadata fields unchanged, and ensure no raw v1 host filesystem path reaches
ProjectRecord.metadata.
In `@docker/reborn/entrypoint.sh`:
- Around line 41-51: Update the migrate command routing in the entrypoint guard
so v1:verify exits through the read-only migration command path instead of
remaining in the mutating whitelist with v1:apply and v1:resume. Preserve the
existing handling for apply/resume while ensuring verify runs target-data checks
without persistent-volume or config-file initialization and without booting the
full Reborn runtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0cc06fdf-c6d7-4b83-9e9f-0f0272bc351d
⛔ Files ignored due to path filters (2)
CHANGELOG.mdis excluded by!CHANGELOG.mdCargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (83)
Dockerfile.rebornFEATURE_PARITY.mdREADME.mdcrates/AGENTS.mdcrates/README.mdcrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/CLAUDE.mdcrates/ironclaw_memory_native/src/backend.rscrates/ironclaw_memory_native/src/lib.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_reborn_cli/AGENTS.mdcrates/ironclaw_reborn_cli/Cargo.tomlcrates/ironclaw_reborn_cli/src/commands/doctor.rscrates/ironclaw_reborn_cli/src/commands/extension.rscrates/ironclaw_reborn_cli/src/commands/migrate.rscrates/ironclaw_reborn_cli/src/commands/mod.rscrates/ironclaw_reborn_cli/src/commands/onboard.rscrates/ironclaw_reborn_cli/src/context.rscrates/ironclaw_reborn_cli/tests/migration_cli.rscrates/ironclaw_reborn_cli/tests/smoke.rscrates/ironclaw_reborn_composition/AGENTS.mdcrates/ironclaw_reborn_composition/CLAUDE.mdcrates/ironclaw_reborn_composition/src/admin_user_directory.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/input.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/migration_support.rscrates/ironclaw_reborn_identity/CONTRACT.mdcrates/ironclaw_reborn_identity/src/filesystem_store/directory.rscrates/ironclaw_reborn_identity/src/filesystem_store/tests.rscrates/ironclaw_reborn_identity/src/lib.rscrates/ironclaw_reborn_identity/src/user_directory.rscrates/ironclaw_reborn_migration/CLAUDE.mdcrates/ironclaw_reborn_migration/Cargo.tomlcrates/ironclaw_reborn_migration/src/convert/automations.rscrates/ironclaw_reborn_migration/src/convert/extensions.rscrates/ironclaw_reborn_migration/src/convert/heartbeat.rscrates/ironclaw_reborn_migration/src/convert/memory.rscrates/ironclaw_reborn_migration/src/convert/mod.rscrates/ironclaw_reborn_migration/src/convert/projects.rscrates/ironclaw_reborn_migration/src/convert/secrets.rscrates/ironclaw_reborn_migration/src/convert/settings.rscrates/ironclaw_reborn_migration/src/convert/threads.rscrates/ironclaw_reborn_migration/src/convert/users.rscrates/ironclaw_reborn_migration/src/error.rscrates/ironclaw_reborn_migration/src/inventory.rscrates/ironclaw_reborn_migration/src/lib.rscrates/ironclaw_reborn_migration/src/main.rscrates/ironclaw_reborn_migration/src/manifest.rscrates/ironclaw_reborn_migration/src/mounts.rscrates/ironclaw_reborn_migration/src/options.rscrates/ironclaw_reborn_migration/src/report.rscrates/ironclaw_reborn_migration/src/source.rscrates/ironclaw_reborn_migration/src/target.rscrates/ironclaw_reborn_migration/src/target_ids.rscrates/ironclaw_reborn_migration/src/v2_model.rscrates/ironclaw_reborn_migration/tests/companion_cli.rscrates/ironclaw_reborn_migration/tests/migration_roundtrip.rscrates/ironclaw_reborn_migration/tests/migration_safety.rscrates/ironclaw_reborn_migration/tests/project_migration.rscrates/ironclaw_secrets/src/filesystem_store.rscrates/ironclaw_secrets/src/lib.rscrates/ironclaw_triggers/src/lib.rscrates/ironclaw_triggers/src/libsql.rscrates/ironclaw_triggers/src/postgres.rscrates/ironclaw_triggers/tests/repository_contract.rsdocker/reborn/entrypoint.shdocs/internal/2026-06-26-legacy-vs-reborn-feature-comparison.mddocs/plans/8513-cli-smoke-and-secrets-decomposition.mddocs/plans/composition-pubuse.snapshotdocs/reborn-binary.mddocs/reborn/README.mddocs/reborn/contracts/memory.mddocs/reborn/contracts/migration-compatibility.mddocs/reborn/contracts/secrets.mddocs/reborn/contracts/triggers.mddocs/reborn/deploy-reborn-cli-docker.mddocs/reborn/onboarding.mddocs/reborn/setup-slack-for-reborn-binary.mddocs/reborn/v1-migration.mdsrc/db/libsql/mod.rssrc/setup/README.md
| serde_json::json!({ | ||
| "schema_version": "ironclaw.reborn.migration-state/v1", | ||
| "migration_protocol_version": 1, | ||
| "release_version": env!("CARGO_PKG_VERSION"), | ||
| "status": "applying", | ||
| }) | ||
| .to_string(), | ||
| ) | ||
| .expect("write marker"); | ||
| let command = ExtensionCommand { | ||
| confirm_host_access: false, | ||
| command: ExtensionSubcommand::Search(ExtensionSearchCommand { | ||
| query: None, | ||
| json: false, | ||
| }), | ||
| }; | ||
|
|
||
| let error = command | ||
| .execute(context) | ||
| .expect_err("quarantine must reject extension lifecycle commands"); | ||
| assert!(error.to_string().contains("quarantined"), "{error:#}"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exercise the applying status instead of a malformed marker.
This JSON omits required binding fields, so deserialization fails and the generic "quarantined" assertion passes before lifecycle policy runs. Create matching local/durable state and assert the applying-status error specifically.
As per path instructions, “Test through the caller” when a helper gates a side effect.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_cli/src/commands/extension.rs` around lines 135 - 155,
Update the quarantine test around ExtensionCommand::execute to create valid
matching local and durable migration state with status “applying,” including all
required binding fields. Invoke the command through ExtensionCommand::execute
and assert the specific applying-status lifecycle error, ensuring the test
exercises policy gating rather than marker deserialization failure.
Source: Path instructions
| #[derive(Debug, Clone, PartialEq, Eq, Deserialize)] | ||
| struct MigrationStateRecord { | ||
| schema_version: String, | ||
| migration_protocol_version: u32, | ||
| run_id: String, | ||
| status: String, | ||
| profile: String, | ||
| target_backend: String, | ||
| target_locator_fingerprint: String, | ||
| tenant_id: String, | ||
| agent_id: String, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Bind quarantine state to the current release.
MigrationStateRecord drops release_version, so local/durable equality can accept a verified claim created by an older release. Read it from both backends and require env!("CARGO_PKG_VERSION"); add a matching-state regression test.
As per coding guidelines, the lifecycle claim must bind “release/protocol, profile, backend, locator fingerprint, tenant, and agent.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_cli/src/commands/migrate.rs` around lines 31 - 42,
Update MigrationStateRecord to include release_version and require it to equal
env!("CARGO_PKG_VERSION") when reading and validating migration state from both
local and durable backends. Ensure equality covers release/protocol, profile,
backend, locator fingerprint, tenant, and agent, and add a regression test
confirming matching current-release state is accepted while older-release state
is rejected.
Source: Coding guidelines
| let target = | ||
| match ironclaw_reborn_composition::resolve_reborn_migration_target(context.boot_config()) { | ||
| Ok(target) => target, | ||
| Err(_) => return Ok(None), | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Propagate target-resolution failures; this currently fails open.
When no local marker exists, converting this error to None lets read_activation_state_status report no quarantine state. Preserve the resolver error with context.
Proposed fix
- Err(_) => return Ok(None),
+ Err(error) => {
+ return Err(error).context(
+ "failed to resolve the Reborn target for migration quarantine inspection",
+ );
+ }As per path instructions, “Fail loud” and propagate errors with context instead of discarding them.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let target = | |
| match ironclaw_reborn_composition::resolve_reborn_migration_target(context.boot_config()) { | |
| Ok(target) => target, | |
| Err(_) => return Ok(None), | |
| }; | |
| let target = | |
| match ironclaw_reborn_composition::resolve_reborn_migration_target(context.boot_config()) { | |
| Ok(target) => target, | |
| Err(error) => { | |
| return Err(error).context( | |
| "failed to resolve the Reborn target for migration quarantine inspection", | |
| ); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_cli/src/commands/migrate.rs` around lines 398 - 402,
Update the target-resolution match in the migration command to propagate
resolver failures instead of converting Err(_) to Ok(None). Add contextual error
information while preserving the existing successful target path and None
behavior only for cases where resolution legitimately returns no target.
Source: Path instructions
| let mut ancestor = normalized.clone(); | ||
| let mut missing_suffix = Vec::new(); | ||
| while !ancestor.exists() { | ||
| let Some(name) = ancestor.file_name() else { | ||
| break; | ||
| }; | ||
| missing_suffix.push(name.to_os_string()); | ||
| if !ancestor.pop() { | ||
| break; | ||
| } | ||
| } | ||
| let mut resolved = ancestor.canonicalize().unwrap_or(ancestor); | ||
| for component in missing_suffix.into_iter().rev() { | ||
| resolved.push(component); | ||
| } | ||
| ironclaw_common::hashing::sha256_hex(resolved.as_os_str().as_encoded_bytes()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Silent canonicalize() fallback risks locator-fingerprint drift.
ancestor.canonicalize().unwrap_or(ancestor) swallows any IO error from canonicalizing an ancestor that was just confirmed to exist (permission error, symlink race, etc.) and silently falls back to the un-canonicalized path. Since this fingerprint is the target identity used for quarantine/activation-guard comparisons and lifecycle-claim binding, an intermittent canonicalize failure on one call and success on another for the same target produces two different fingerprints for the same physical database — defeating the "atomic, run-bound... locator fingerprint" guarantee this function exists to provide.
Propagate the canonicalize error into RebornBuildError::InvalidConfig instead of silently falling back, or add an explicit // silent-ok: <reason> comment if the fallback is intentionally acceptable.
🔧 Proposed fix
- let mut resolved = ancestor.canonicalize().unwrap_or(ancestor);
+ let resolved_ancestor = ancestor.canonicalize().map_err(|error| {
+ // propagate to caller; this fn signature would need to become fallible
+ error
+ })?;
+ let mut resolved = resolved_ancestor;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_composition/src/migration_support.rs` around lines 195
- 211, Update the locator-fingerprint resolution around ancestor
canonicalization to propagate failures from canonicalize instead of using
unwrap_or(ancestor). Convert the error into RebornBuildError::InvalidConfig
through the enclosing function’s existing error path, while preserving the
missing-suffix reconstruction and hashing behavior after successful
canonicalization.
Source: Coding guidelines
| #[test] | ||
| fn lifecycle_help_never_accepts_raw_postgres_urls_or_keys() { | ||
| let output = Command::new(companion_bin()) | ||
| .args(["v1", "plan", "--help"]) | ||
| .env_clear() | ||
| .output() | ||
| .expect("run migration companion help"); | ||
|
|
||
| assert!(output.status.success()); | ||
| let stdout = String::from_utf8_lossy(&output.stdout); | ||
| assert!(stdout.contains("--source-postgres"), "stdout: {stdout}"); | ||
| assert!(stdout.contains("--source-home"), "stdout: {stdout}"); | ||
| assert!( | ||
| !stdout.contains("--source-postgres-url"), | ||
| "stdout: {stdout}" | ||
| ); | ||
| assert!(!stdout.contains("--target-postgres"), "stdout: {stdout}"); | ||
| assert!(!stdout.contains("--secret-master-key"), "stdout: {stdout}"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Cover every source-bearing lifecycle subcommand.
This only inspects v1 plan --help; apply or resume could expose raw URL/key flags without failing the test. Apply the forbidden-flag assertions to every relevant subcommand.
As per coding guidelines, “Do not accept PostgreSQL source URLs or source keys as raw CLI values.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_migration/tests/companion_cli.rs` around lines 36 -
54, Expand lifecycle_help_never_accepts_raw_postgres_urls_or_keys to invoke help
for every relevant v1 lifecycle subcommand, including plan, apply, and resume.
Assert each command exposes the supported source flags and rejects
--source-postgres-url, --target-postgres, and --secret-master-key, preserving
the existing success and output diagnostics.
Source: Coding guidelines
| // A stale applied manifest cannot reopen a verified target claim or | ||
| // overwrite operator/runtime state. | ||
| overwrite_trigger_prompt(&dst, "cron-light", "divergent target prompt").await; | ||
| let resume_manifest = report2.manifest.as_ref().expect("resume manifest"); | ||
| let collision = resume_migration( | ||
| options(src, dst.clone(), false), | ||
| resume_manifest, | ||
| MigrationSecretInputs { | ||
| source_master_key: Some(SecretString::from(MASTER_KEY)), | ||
| target_master_key: None, | ||
| }, | ||
| ApplyAcknowledgements::offline_snapshot(), | ||
| ) | ||
| .await | ||
| .expect_err("verified target claim must fail closed"); | ||
| assert!( | ||
| collision | ||
| .to_string() | ||
| .contains("invalid shared migration state transition verified -> applying"), | ||
| "unexpected collision error: {collision}" | ||
| ); | ||
| assert_eq!( | ||
| installations[0]["owner"]["user_ids"], | ||
| serde_json::json!([USER, USER_BOB]) | ||
| reborn_triggers(&dst) | ||
| .await | ||
| .into_iter() | ||
| .find(|trigger| trigger.name == "cron-light") | ||
| .expect("cron-light trigger") | ||
| .prompt, | ||
| "divergent target prompt", | ||
| "migration must not overwrite a divergent deterministic slot" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This path never exercises trigger collision reconciliation.
After verification, resume fails at verified -> applying, before the divergent trigger is read. Add a separate resume from an allowed lifecycle state and assert the divergent-slot error plus unchanged prompt.
As per coding guidelines, divergent target collisions must fail without overwriting and side-effect gates must be tested through the caller.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_migration/tests/migration_roundtrip.rs` around lines
952 - 982, The test currently expects the stale manifest to fail during the
lifecycle transition before trigger collision reconciliation runs. Add a
separate resume scenario using an allowed migration state, invoke
resume_migration through the normal caller, and assert the divergent target-slot
collision error. Keep the existing assertion that the “cron-light” prompt
remains “divergent target prompt,” verifying the collision fails without
overwriting.
Source: Coding guidelines
| async fn plan_is_source_read_only_and_does_not_create_target() { | ||
| let directory = tempfile::tempdir().expect("tempdir"); | ||
| let source = directory.path().join("source-with-password-canary.db"); | ||
| let target = directory.path().join("new-reborn-home").join("target.db"); | ||
| seed_source(&source).await; | ||
| let before = source_contents(&source).await; | ||
| let bytes_before = std::fs::read(&source).expect("read source snapshot"); | ||
|
|
||
| let manifest = plan_migration(&options(source.clone(), target.clone())) | ||
| .await | ||
| .expect("plan"); | ||
|
|
||
| assert_eq!(manifest.status, MigrationStatus::Planned); | ||
| assert_eq!(source_contents(&source).await, before); | ||
| assert_eq!( | ||
| std::fs::read(&source).expect("read source after plan"), | ||
| bytes_before, | ||
| "planning modified source database bytes" | ||
| ); | ||
| assert!(!target.exists(), "planning created the target database"); | ||
| assert!( | ||
| !target.parent().expect("target parent").exists(), | ||
| "planning created the target home" | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Snapshot the whole source store, not only the main DB file.
source_contents() reopens libSQL, while the byte check ignores -wal, -shm, and other sidecars. Snapshot relative paths and bytes for the entire source directory before and after planning without reopening it.
As per coding guidelines, source readers must be read-only and migration must preserve source immutability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_migration/tests/migration_safety.rs` around lines 162
- 185, Update plan_is_source_read_only_and_does_not_create_target to snapshot
every file under the source store directory, including relative paths and bytes,
before plan_migration and compare the complete snapshot afterward. Avoid
source_contents() or any reopening of the source during verification, and retain
the assertions that planning does not create the target database or parent
directory.
Source: Coding guidelines
| /// Atomically inserts a secret only when its deterministic slot is absent. | ||
| /// | ||
| /// An existing record is never replaced. Instead, the store compares its | ||
| /// expiry and material without exposing the stored value and reports | ||
| /// whether it is an exact match or divergent state. | ||
| async fn put_if_absent_or_matches( | ||
| &self, | ||
| scope: ResourceScope, | ||
| handle: SecretHandle, | ||
| material: SecretMaterial, | ||
| expires_at: Option<Timestamp>, | ||
| ) -> Result<SecretPutOutcome, SecretStoreError> { | ||
| let _ = (scope, handle, material, expires_at); | ||
| Err(SecretStoreError::StoreUnavailable { | ||
| reason: "atomic secret insertion is unavailable".to_string(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP -C3 'impl(?:<[^>]+>)?\s+SecretStore\s+for' crates -g '*.rs'
ast-grep run \
--pattern 'async fn put_if_absent_or_matches($$$) { $$$ }' \
--lang rust cratesRepository: nearai/ironclaw
Length of output: 9475
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the SecretStore trait and the new default method.
sed -n '880,1075p' crates/ironclaw_secrets/src/lib.rs
# List every SecretStore impl and whether it mentions the new method.
rg -n -C2 'impl(?:<[^>]+>)?\s+SecretStore\s+for|put_if_absent_or_matches\(' crates -g '*.rs'Repository: nearai/ironclaw
Length of output: 16257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the wrapper and in-memory implementations around the trait impls.
sed -n '1210,1285p' crates/ironclaw_secrets/src/lib.rs
sed -n '1490,1565p' crates/ironclaw_secrets/src/lib.rs
sed -n '340,455p' crates/ironclaw_secrets/src/filesystem_store.rs
# Check where put_if_absent_or_matches is called in production code.
rg -n -C2 'put_if_absent_or_matches\(' crates src -g '*.rs'Repository: nearai/ironclaw
Length of output: 11283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the migration wiring around the atomic insert call.
sed -n '1,260p' crates/ironclaw_reborn_migration/src/convert/secrets.rs
# Find constructors/usages of the scoped adapter and in-memory store.
rg -n -C3 'ScopedSecretsStoreAdapter|InMemorySecretStore::new|InMemorySecretStore::default|put_if_absent_or_matches\(' crates -g '*.rs'Repository: nearai/ironclaw
Length of output: 50372
Forward put_if_absent_or_matches in the wrapper stores ScopedSecretsStoreAdapter<S> and InMemorySecretStore still inherit the StoreUnavailable default in crates/ironclaw_secrets/src/lib.rs, so any caller through those paths loses atomic-insert semantics. Make the method required or delegate it in each wrapper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_secrets/src/lib.rs` around lines 1021 - 1037, Implement
put_if_absent_or_matches for both ScopedSecretsStoreAdapter<S> and
InMemorySecretStore instead of inheriting the StoreUnavailable default. Delegate
through ScopedSecretsStoreAdapter to its wrapped store with the appropriate
scope handling, and add the equivalent atomic insert, expiry, and material-match
behavior to InMemorySecretStore so callers preserve SecretPutOutcome semantics.
Source: Coding guidelines
| cargo run -q -p ironclaw_reborn_cli --bin ironclaw-reborn -- onboard --migrate-v1 | ||
| ``` | ||
|
|
||
| `--dry-run` reports what would be initialized without writing files. | ||
| `--import-history` reserves the history-import step in the summary (not wired | ||
| yet). See `docs/reborn/onboarding.md` for the full slice description and the | ||
| completion-marker schema. | ||
| `--migrate-v1` explicitly runs only the migration planning step after detecting | ||
| a source; it never applies automatically. `--import-history` is a deprecated | ||
| hidden alias. See `docs/reborn/onboarding.md` for the completion-marker schema | ||
| and `docs/reborn/v1-migration.md` for cutover and rollback. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify that migration onboarding reads v1 state.
Lines 399-402 introduce --migrate-v1, but the section above says onboarding does not call into v1 import state. Scope that claim to normal onboarding or explicitly exempt read-only migration planning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/reborn-binary.md` around lines 395 - 402, Clarify the onboarding
documentation’s statement about v1 import state so it applies only to normal
onboarding. Explicitly exempt the read-only --migrate-v1 migration planning
path, which reads detected v1 state without applying changes, while preserving
the existing behavior descriptions for --dry-run and --import-history.
…get` to Reborn (#4379) * feat: migrate read-only commands `doctor`, `status` and `config list/get` to Reborn * fix: fix bug when `llm.default.` placeholders were omiitted * refactor: refactor `Renderable` to write to a `Write` instead of directly to stdout * test: expand unit test coverage * fix: include `slack.enabled` config keys in `config` output by refactoring `flatten_config` * chore: suppress `check_no_panics` with safety comment * fix: get rid of unnecessary panic in `flatten_config` * fix: use `is_dir` instead of `exists` for `reborn_home` check * fix: prevent race condition when file could be deleted between syscalls * test: add smoke test for `doctor --json` output * test: add test for negative integer coercion to float in config values * test: check all three outcome icons are rendered in `doctor` output * test: test `build_config_get_dto` when it returns `Some(value)` * test: remove duplicae `test_context` function * test: tighten `doctor_reports_explicit_profile` to perform same-line check for "profile" and "production" in stdout * test: use `IgnoredAny` instead of `Value` for better perf * test: test `Failed` branch of `driver_check` * test: additionally test `collect_leaf_entries` with array values * fix: ensure non-string elements aren't lost in `collect_leaf_entries` * chore: document chosen trade-off in `build_config_get_dto` * style: fix formatting * chore: fix clippy warning, add `Serialize` to `SlackChannelRouteSection` * chore: skip serializing if `channel_routes` is empty * fix: address Reborn CLI review findings * fix: gate provider diagnostics with LLM feature * test: update composition facade snapshot * fix: keep CLI rendering before test modules --------- Co-authored-by: Firat Sertgoz <f@nuff.tech>
9e41fe2 to
9f215c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/factory.rs (1)
1253-1280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a RebornServices-level regression for lifecycle auth attachment
build_reborn_serviceswiresLifecycleProductFacadeSlotafter product auth is created, so the tests incrates/ironclaw_reborn_composition/src/lifecycle_auth_continuation.rsstill miss the real attachment path. Add one test that exercises auth completion after the slot is attached, and one that keeps lifecycle support absent so the path stays fail-closed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_composition/src/factory.rs` around lines 1253 - 1280, Add RebornServices-level regression tests around build_reborn_services, covering auth completion after LifecycleProductFacadeSlot is attached and confirming lifecycle support remains fail-closed when absent. Exercise the actual factory wiring through the returned service, rather than only testing lifecycle_auth_continuation.rs components in isolation, and preserve existing product-auth behavior in both cases.Sources: Coding guidelines, Path instructions
♻️ Duplicate comments (4)
crates/ironclaw_reborn_migration/src/convert/secrets.rs (1)
175-181: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not expose
SecretStoreErrorthrough the migration CLI.
e.to_string()still places backend internals in the operator-facingreason. Use a fixed safe message, consistent withsecret_collision.This repeats the earlier unresolved review finding. As per coding guidelines, raw backend paths, SQL, and transport internals must not cross public surfaces.
Proposed fix
- .map_err(|e| MigrationError::WriteTarget { + .map_err(|_| MigrationError::WriteTarget { domain: format!("secret {user_id}:{name}"), - reason: e.to_string(), + reason: "failed to write deterministic target secret slot".to_string(), })?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_migration/src/convert/secrets.rs` around lines 175 - 181, Update the error mapping around put_if_absent_or_matches in the migration conversion flow to replace e.to_string() with the same fixed safe message used by secret_collision. Keep backend SecretStoreError details out of MigrationError::WriteTarget.reason while preserving the existing domain value.Source: Coding guidelines
docker/reborn/entrypoint.sh (1)
37-50: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
verifyread-only at the entrypoint.
v1:verifystill falls through to normal initialization and config seeding, allowing target mutation before verification. Onlyapplyandresumeshould enter the mutating path.This repeats the earlier unresolved review finding and conflicts with the PR’s strict verification contract.
Proposed fix
-# seed config first. Mutating/resume/verify operations continue through the +# seed config first. Mutating/resume operations continue through the # normal persistent-volume and config checks below. @@ - v1:apply|v1:resume|v1:verify) ;; + v1:apply|v1:resume) ;;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker/reborn/entrypoint.sh` around lines 37 - 50, Update the migration dispatch condition in the entrypoint so only v1:apply and v1:resume continue into the mutating initialization path; route v1:verify through the immediate exec path alongside read-only planning, status, and help operations, preserving argument handling for other migration commands.crates/ironclaw_reborn_migration/src/convert/projects.rs (1)
71-77: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not persist the legacy host workspace path.
workspace_pathcan expose an absolute source-host path through canonical project metadata. Remove it or replace it with a non-path migration marker.This repeats the earlier unresolved review finding. As per coding guidelines, raw backend paths must not cross public surfaces.
Proposed fix
"legacy_engine_v2": { "goals": project.goals, "metrics": project.metrics, "metadata": project.metadata, - "workspace_path": project.workspace_path, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_migration/src/convert/projects.rs` around lines 71 - 77, Remove workspace_path from the legacy_engine_v2 metadata constructed in the project conversion flow, using the surrounding metadata fields in the projects conversion implementation as the change point. Do not persist or expose the host filesystem path; retain the other legacy project fields unchanged.Source: Coding guidelines
crates/ironclaw_reborn_migration/src/convert/memory.rs (1)
38-48: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the owning converters’ exact path classifiers.
ends_with("mission.json")andcontains("/threads/")still classify unrelated runtime paths as supported, causing them to be skipped without a loss disposition. The tests also omit the previously identified mission/thread near matches. Reuse shared exact classifiers and add negative coverage for nestedmission.jsonand unrelated/threads/paths.This repeats the earlier unresolved review finding. The migration contract requires every unsupported artifact to receive an explicit disposition.
Also applies to: 175-199
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_migration/src/convert/memory.rs` around lines 38 - 48, Replace the broad path checks used by the engine migration flow with the owning converters’ exact shared classifiers, including the logic behind v2_model::is_engine_path and has_engine_converter. Ensure nested mission.json files and unrelated /threads/ paths are not treated as supported, and record an explicit LossReason::NoTargetConcept disposition before continuing. Extend the relevant tests with these negative near-match cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_memory_native/src/backend.rs`:
- Around line 1201-1215: Add a caller-level async test alongside
metadata_capability_rejects_direct_backend_metadata_reads that invokes
read_document_metadata with alpha_context() and beta_path(), then asserts the
operation fails before any cross-scope repository read occurs. Keep the
capability setup enabled for metadata so the failure specifically verifies
ensure_path_matches_context scope enforcement.
In `@crates/ironclaw_reborn_cli/Cargo.toml`:
- Line 92: Update the optional libsql dependency declaration to retain only the
features required by the CLI’s local Builder::new_local and
OpenFlags::SQLITE_OPEN_READ_ONLY usage, removing replication, remote, and tls
unless another consumer in this crate demonstrably requires them.
In `@crates/ironclaw_reborn_composition/src/lib.rs`:
- Around line 923-937: Update open_reborn_postgres_pool_with_max_size to resolve
TLS settings through input::postgres_pool_tls_options_from_env(), mapping
failures to RebornCompositionError::InvalidConfig like
open_reborn_postgres_pool. Pass the resolved tls_options to
ironclaw_reborn_event_store::open_postgres_pool_with_tls_options(url, max_size,
tls_options) instead of the default-TLS helper.
In `@crates/ironclaw_reborn_migration/src/convert/automations.rs`:
- Around line 521-529: Update the thread migration flow around thread_project_id
to detect when mission.project_id is present but excluded because it is not in
imported_project_ids, and call record_loss for that dropped project scope.
Mirror the degraded-migration handling used by resolve_mission_project(),
ensuring MigrationReport records the loss before continuing with thread imports.
In `@crates/ironclaw_reborn_migration/src/convert/projects.rs`:
- Around line 47-56: Update the project ownership handling before calling
report.valid_user_id in the project conversion flow: when both project.user_id
and document.user_id are present, require them to match; on a mismatch, record
the ownership loss through the existing report mechanism and skip the project.
Preserve the current valid_user_id validation for non-conflicting ownership and
ensure migrated records retain the document’s tenant/user scope.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 1253-1280: Add RebornServices-level regression tests around
build_reborn_services, covering auth completion after LifecycleProductFacadeSlot
is attached and confirming lifecycle support remains fail-closed when absent.
Exercise the actual factory wiring through the returned service, rather than
only testing lifecycle_auth_continuation.rs components in isolation, and
preserve existing product-auth behavior in both cases.
---
Duplicate comments:
In `@crates/ironclaw_reborn_migration/src/convert/memory.rs`:
- Around line 38-48: Replace the broad path checks used by the engine migration
flow with the owning converters’ exact shared classifiers, including the logic
behind v2_model::is_engine_path and has_engine_converter. Ensure nested
mission.json files and unrelated /threads/ paths are not treated as supported,
and record an explicit LossReason::NoTargetConcept disposition before
continuing. Extend the relevant tests with these negative near-match cases.
In `@crates/ironclaw_reborn_migration/src/convert/projects.rs`:
- Around line 71-77: Remove workspace_path from the legacy_engine_v2 metadata
constructed in the project conversion flow, using the surrounding metadata
fields in the projects conversion implementation as the change point. Do not
persist or expose the host filesystem path; retain the other legacy project
fields unchanged.
In `@crates/ironclaw_reborn_migration/src/convert/secrets.rs`:
- Around line 175-181: Update the error mapping around put_if_absent_or_matches
in the migration conversion flow to replace e.to_string() with the same fixed
safe message used by secret_collision. Keep backend SecretStoreError details out
of MigrationError::WriteTarget.reason while preserving the existing domain
value.
In `@docker/reborn/entrypoint.sh`:
- Around line 37-50: Update the migration dispatch condition in the entrypoint
so only v1:apply and v1:resume continue into the mutating initialization path;
route v1:verify through the immediate exec path alongside read-only planning,
status, and help operations, preserving argument handling for other migration
commands.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d5e4f0c5-fdd7-4b1f-972b-ff67a9606231
⛔ Files ignored due to path filters (2)
CHANGELOG.mdis excluded by!CHANGELOG.mdCargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (83)
Dockerfile.rebornFEATURE_PARITY.mdREADME.mdcrates/AGENTS.mdcrates/README.mdcrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/CLAUDE.mdcrates/ironclaw_memory_native/src/backend.rscrates/ironclaw_memory_native/src/lib.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_reborn_cli/AGENTS.mdcrates/ironclaw_reborn_cli/Cargo.tomlcrates/ironclaw_reborn_cli/src/commands/doctor.rscrates/ironclaw_reborn_cli/src/commands/extension.rscrates/ironclaw_reborn_cli/src/commands/migrate.rscrates/ironclaw_reborn_cli/src/commands/mod.rscrates/ironclaw_reborn_cli/src/commands/onboard.rscrates/ironclaw_reborn_cli/src/context.rscrates/ironclaw_reborn_cli/tests/migration_cli.rscrates/ironclaw_reborn_cli/tests/smoke.rscrates/ironclaw_reborn_composition/AGENTS.mdcrates/ironclaw_reborn_composition/CLAUDE.mdcrates/ironclaw_reborn_composition/src/admin_user_directory.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/input.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/migration_support.rscrates/ironclaw_reborn_identity/CONTRACT.mdcrates/ironclaw_reborn_identity/src/filesystem_store/directory.rscrates/ironclaw_reborn_identity/src/filesystem_store/tests.rscrates/ironclaw_reborn_identity/src/lib.rscrates/ironclaw_reborn_identity/src/user_directory.rscrates/ironclaw_reborn_migration/CLAUDE.mdcrates/ironclaw_reborn_migration/Cargo.tomlcrates/ironclaw_reborn_migration/src/convert/automations.rscrates/ironclaw_reborn_migration/src/convert/extensions.rscrates/ironclaw_reborn_migration/src/convert/heartbeat.rscrates/ironclaw_reborn_migration/src/convert/memory.rscrates/ironclaw_reborn_migration/src/convert/mod.rscrates/ironclaw_reborn_migration/src/convert/projects.rscrates/ironclaw_reborn_migration/src/convert/secrets.rscrates/ironclaw_reborn_migration/src/convert/settings.rscrates/ironclaw_reborn_migration/src/convert/threads.rscrates/ironclaw_reborn_migration/src/convert/users.rscrates/ironclaw_reborn_migration/src/error.rscrates/ironclaw_reborn_migration/src/inventory.rscrates/ironclaw_reborn_migration/src/lib.rscrates/ironclaw_reborn_migration/src/main.rscrates/ironclaw_reborn_migration/src/manifest.rscrates/ironclaw_reborn_migration/src/mounts.rscrates/ironclaw_reborn_migration/src/options.rscrates/ironclaw_reborn_migration/src/report.rscrates/ironclaw_reborn_migration/src/source.rscrates/ironclaw_reborn_migration/src/target.rscrates/ironclaw_reborn_migration/src/target_ids.rscrates/ironclaw_reborn_migration/src/v2_model.rscrates/ironclaw_reborn_migration/tests/companion_cli.rscrates/ironclaw_reborn_migration/tests/migration_roundtrip.rscrates/ironclaw_reborn_migration/tests/migration_safety.rscrates/ironclaw_reborn_migration/tests/project_migration.rscrates/ironclaw_secrets/src/filesystem_store.rscrates/ironclaw_secrets/src/lib.rscrates/ironclaw_triggers/src/lib.rscrates/ironclaw_triggers/src/libsql.rscrates/ironclaw_triggers/src/postgres.rscrates/ironclaw_triggers/tests/repository_contract.rsdocker/reborn/entrypoint.shdocs/internal/2026-06-26-legacy-vs-reborn-feature-comparison.mddocs/plans/8513-cli-smoke-and-secrets-decomposition.mddocs/plans/composition-pubuse.snapshotdocs/reborn-binary.mddocs/reborn/README.mddocs/reborn/contracts/memory.mddocs/reborn/contracts/migration-compatibility.mddocs/reborn/contracts/secrets.mddocs/reborn/contracts/triggers.mddocs/reborn/deploy-reborn-cli-docker.mddocs/reborn/onboarding.mddocs/reborn/setup-slack-for-reborn-binary.mddocs/reborn/v1-migration.mdsrc/db/libsql/mod.rssrc/setup/README.md
| #[tokio::test] | ||
| async fn metadata_capability_rejects_direct_backend_metadata_reads() { | ||
| let backend = make_backend().with_capabilities( | ||
| MemoryBackendCapabilities::default() | ||
| .set_file_documents(true) | ||
| .set_metadata(false), | ||
| ); | ||
| let result = backend | ||
| .read_document_metadata(&alpha_context(), &alpha_path()) | ||
| .await; | ||
| assert!( | ||
| result.is_err(), | ||
| "metadata reads must fail closed when metadata is unsupported" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add a scope-mismatch regression for metadata reads.
The new operation also gates repository access through ensure_path_matches_context, but this test covers only capability denial. Add an alpha_context()/beta_path() test proving no cross-scope metadata read reaches the repository.
As per coding guidelines, “when a helper gates a side effect… add a caller-level test.” As per path instructions, “Test through the caller.”
Proposed regression test
+ #[tokio::test]
+ async fn read_document_metadata_rejects_path_outside_authorized_context() {
+ let backend = make_backend();
+ let result = backend
+ .read_document_metadata(&alpha_context(), &beta_path())
+ .await;
+ assert!(
+ result.is_err(),
+ "expected scope mismatch on metadata read to fail closed"
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[tokio::test] | |
| async fn metadata_capability_rejects_direct_backend_metadata_reads() { | |
| let backend = make_backend().with_capabilities( | |
| MemoryBackendCapabilities::default() | |
| .set_file_documents(true) | |
| .set_metadata(false), | |
| ); | |
| let result = backend | |
| .read_document_metadata(&alpha_context(), &alpha_path()) | |
| .await; | |
| assert!( | |
| result.is_err(), | |
| "metadata reads must fail closed when metadata is unsupported" | |
| ); | |
| } | |
| #[tokio::test] | |
| async fn metadata_capability_rejects_direct_backend_metadata_reads() { | |
| let backend = make_backend().with_capabilities( | |
| MemoryBackendCapabilities::default() | |
| .set_file_documents(true) | |
| .set_metadata(false), | |
| ); | |
| let result = backend | |
| .read_document_metadata(&alpha_context(), &alpha_path()) | |
| .await; | |
| assert!( | |
| result.is_err(), | |
| "metadata reads must fail closed when metadata is unsupported" | |
| ); | |
| } | |
| #[tokio::test] | |
| async fn read_document_metadata_rejects_path_outside_authorized_context() { | |
| let backend = make_backend(); | |
| let result = backend | |
| .read_document_metadata(&alpha_context(), &beta_path()) | |
| .await; | |
| assert!( | |
| result.is_err(), | |
| "expected scope mismatch on metadata read to fail closed" | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_memory_native/src/backend.rs` around lines 1201 - 1215, Add a
caller-level async test alongside
metadata_capability_rejects_direct_backend_metadata_reads that invokes
read_document_metadata with alpha_context() and beta_path(), then asserts the
operation fails before any cross-scope repository read occurs. Keep the
capability setup enabled for metadata so the failure specifically verifies
ensure_path_matches_context scope enforcement.
Sources: Coding guidelines, Path instructions
| ironclaw_reborn_traces = { path = "../ironclaw_reborn_traces", version = "0.1.0" } | ||
| ironclaw_reborn_webui_ingress = { path = "../ironclaw_reborn_webui_ingress", version = "0.1.0", optional = true } | ||
| reqwest = { version = "0.12", default-features = false, features = ["json", "multipart", "rustls-tls-native-roots", "stream"] } | ||
| libsql = { version = "0.9", optional = true, default-features = false, features = ["core", "replication", "remote", "tls"] } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'libsql::(Builder|OpenFlags|Database|Connection|replication|Cipher|remote|sync)' crates/ironclaw_reborn_cli/srcRepository: nearai/ironclaw
Length of output: 381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== repo guidance files ==\n'
fd -a 'CLAUDE.md|AGENTS.md|rules' . -t f
printf '\n== Cargo.toml context ==\n'
sed -n '1,180p' crates/ironclaw_reborn_cli/Cargo.toml
printf '\n== migrate.rs libsql context ==\n'
sed -n '450,540p' crates/ironclaw_reborn_cli/src/commands/migrate.rs
printf '\n== libsql references in CLI crate ==\n'
rg -n 'libsql' crates/ironclaw_reborn_cli/src crates/ironclaw_reborn_cli/Cargo.tomlRepository: nearai/ironclaw
Length of output: 19228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== root AGENTS ==\n'
cat -n AGENTS.md
printf '\n== crate AGENTS ==\n'
cat -n crates/ironclaw_reborn_cli/AGENTS.md
printf '\n== nearby migration docs ==\n'
fd -a 'CLAUDE.md|AGENTS.md' crates/ironclaw_reborn_migration crates/ironclaw_reborn_cli crates/ironclaw_reborn_compositionRepository: nearai/ironclaw
Length of output: 18531
🌐 Web query:
libsql 0.9 Cargo features core replication remote tls Builder::new_local OpenFlags SQLITE_OPEN_READ_ONLY docs
💡 Result:
In the libsql Rust crate (version 0.9.x), feature flags are used to manage dependencies and reduce compile times [1]. Key features include: - core: Enables the core C code required for local database operations and embedded replicas [1][2]. It is enabled by default [3]. - replication: Includes the core feature and adds HTTP-based replication logic to sync remote databases locally [1]. It is enabled by default [3]. - remote: Provides pure HTTP functionality for executing queries against a remote database [1]. It is enabled by default [3]. - tls: Enables built-in TLS support via hyper-rustls. If disabled, you must provide your own TLS connector for features requiring HTTP [1][2]. It is enabled by default [3]. The Builder struct is the recommended way to construct a Database instance [4][5]. The Builder::new_local(path) method creates a builder for a local-only database that does not perform networking or connect to remote servers [6][7]. This method requires the core feature to be enabled [6]. For database opening modes, the OpenFlags struct (provided by the core feature) allows you to specify flags such as SQLITE_OPEN_READ_ONLY, SQLITE_OPEN_READ_WRITE, and SQLITE_OPEN_CREATE [8][4][9]. While older Database::open methods are deprecated in favor of the Builder pattern, OpenFlags remains the standard mechanism for defining access modes [4][9]. Note that the default flags for a database are SQLITE_OPEN_READ_WRITE | SQLITE_OPEN_CREATE [4][9].
Citations:
- 1: https://docs.rs/libsql/latest/libsql/
- 2: https://crates.io/crates/libsql/0.9.29
- 3: https://docs.rs/crate/libsql/latest/features
- 4: https://docs.rs/libsql/latest/src/libsql/database.rs.html
- 5: https://docs.rs/libsql/latest/libsql/struct.Database.html
- 6: https://docs.rs/libsql/latest/libsql/struct.Builder.html
- 7: https://github.com/tursodatabase/libsql/blob/b5dab26b/libsql/src/database/builder.rs
- 8: https://docs.rs/libsql/latest/libsql/struct.OpenFlags.html
- 9: https://github.com/tursodatabase/libsql/blob/b5dab26b/libsql/src/database.rs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== root AGENTS ==\n'
cat -n AGENTS.md
printf '\n== crate AGENTS ==\n'
cat -n crates/ironclaw_reborn_cli/AGENTS.md
printf '\n== nearby guidance files ==\n'
fd -a 'CLAUDE.md|AGENTS.md' crates/ironclaw_reborn_migration crates/ironclaw_reborn_cli crates/ironclaw_reborn_compositionRepository: nearai/ironclaw
Length of output: 18531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== runtime libsql context A ==\n'
sed -n '1228,1260p' crates/ironclaw_reborn_cli/src/runtime/mod.rs
printf '\n== runtime libsql context B ==\n'
sed -n '1908,1942p' crates/ironclaw_reborn_cli/src/runtime/mod.rs
printf '\n== runtime libsql context C ==\n'
sed -n '2230,2262p' crates/ironclaw_reborn_cli/src/runtime/mod.rsRepository: nearai/ironclaw
Length of output: 4233
Trim the libSQL feature set crates/ironclaw_reborn_cli/AGENTS.md says to keep the CLI small and explicit. This crate only uses libsql::Builder::new_local plus OpenFlags::SQLITE_OPEN_READ_ONLY; replication, remote, and tls don’t show up on this path. Drop them unless another consumer needs the networked libSQL code paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_cli/Cargo.toml` at line 92, Update the optional libsql
dependency declaration to retain only the features required by the CLI’s local
Builder::new_local and OpenFlags::SQLITE_OPEN_READ_ONLY usage, removing
replication, remote, and tls unless another consumer in this crate demonstrably
requires them.
| pub fn open_reborn_postgres_pool( | ||
| url: secrecy::SecretString, | ||
| ) -> Result<deadpool_postgres::Pool, RebornCompositionError> { | ||
| Ok(ironclaw_reborn_event_store::open_postgres_pool(url)?) | ||
| let tls_options = input::postgres_pool_tls_options_from_env().map_err(|error| { | ||
| RebornCompositionError::InvalidConfig { | ||
| reason: error.to_string(), | ||
| } | ||
| })?; | ||
| Ok( | ||
| ironclaw_reborn_event_store::open_postgres_pool_with_tls_options( | ||
| url, | ||
| ironclaw_reborn_event_store::DEFAULT_POSTGRES_POOL_MAX_SIZE, | ||
| tls_options, | ||
| )?, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 \
'fn open_postgres_pool_with_(tls_options|max_size)|open_reborn_postgres_pool_with_max_size' \
cratesRepository: nearai/ironclaw
Length of output: 2477
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '900,960p' crates/ironclaw_reborn_composition/src/lib.rs
printf '\n---\n'
sed -n '80,130p' crates/ironclaw_reborn_event_store/src/lib.rs
printf '\n---\n'
rg -n -C3 'postgres_pool_tls_options_from_env|open_postgres_pool_with_tls_options|open_postgres_pool_with_max_size' crates/ironclaw_reborn_composition crates/ironclaw_reborn_event_storeRepository: nearai/ironclaw
Length of output: 9002
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'open_reborn_postgres_pool_with_max_size\(' crates src testsRepository: nearai/ironclaw
Length of output: 800
Apply the env-derived TLS policy here too.
open_reborn_postgres_pool_with_max_size still calls open_postgres_pool_with_max_size, which uses Default::default() TLS options. That leaves a public production storage entrypoint on a different TLS path than open_reborn_postgres_pool. Resolve postgres_pool_tls_options_from_env() here as well and call open_postgres_pool_with_tls_options(url, max_size, tls_options).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_composition/src/lib.rs` around lines 923 - 937, Update
open_reborn_postgres_pool_with_max_size to resolve TLS settings through
input::postgres_pool_tls_options_from_env(), mapping failures to
RebornCompositionError::InvalidConfig like open_reborn_postgres_pool. Pass the
resolved tls_options to
ironclaw_reborn_event_store::open_postgres_pool_with_tls_options(url, max_size,
tls_options) instead of the default-TLS helper.
| let thread_project_id = mission | ||
| .project_id | ||
| .filter(|id| imported_project_ids.contains(id.to_string().as_str())); | ||
| let mut migrated_thread_ids = BTreeSet::new(); | ||
| for tid in &mission.thread_history { | ||
| let Some(thread) = engine_threads.get(tid) else { | ||
| if !migrated_thread_ids.insert(*tid) { | ||
| continue; | ||
| } | ||
| let Some(indexed_thread) = engine_threads.get(tid) else { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'project_id' crates/ironclaw_reborn_migration/src/convert/automations.rs | rg -n 'record_loss|record_mission_field_losses|fn record_mission_field_losses'
ast-grep run --pattern 'fn record_mission_field_losses($$$) { $$$ }' --lang rust crates/ironclaw_reborn_migration/src/convert/automations.rsRepository: nearai/ironclaw
Length of output: 1884
🏁 Script executed:
#!/bin/bash
sed -n '430,540p' crates/ironclaw_reborn_migration/src/convert/automations.rs
printf '\n---\n'
sed -n '740,780p' crates/ironclaw_reborn_migration/src/convert/automations.rs
printf '\n---\n'
rg -n 'resolve_mission_project|record_mission_field_losses|project_id|LossReason::Degraded|record_loss' crates/ironclaw_reborn_migration/src/convert/automations.rsRepository: nearai/ironclaw
Length of output: 8734
🏁 Script executed:
#!/bin/bash
sed -n '521,575p' crates/ironclaw_reborn_migration/src/convert/automations.rs
printf '\n---\n'
sed -n '699,730p' crates/ironclaw_reborn_migration/src/convert/automations.rs
printf '\n---\n'
rg -n 'MissionThread|thread_project_id|project_id:' crates/ironclaw_reborn_migration/src/convert/automations.rsRepository: nearai/ironclaw
Length of output: 3588
Record a loss when thread project scope is dropped
thread_project_id filters out an unimported mission.project_id without calling record_loss, so thread imports can lose project scope silently. resolve_mission_project() already treats this as a degraded migration for trigger records; mirror that here so the MigrationReport accounts for the missing scope too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_migration/src/convert/automations.rs` around lines 521
- 529, Update the thread migration flow around thread_project_id to detect when
mission.project_id is present but excluded because it is not in
imported_project_ids, and call record_loss for that dropped project scope.
Mirror the degraded-migration handling used by resolve_mission_project(),
ensuring MigrationReport records the loss before continuing with thread imports.
| let owner_raw = if project.user_id.is_empty() { | ||
| document.user_id.as_str() | ||
| } else { | ||
| project.user_id.as_str() | ||
| }; | ||
| let Some(owner_user_id) = | ||
| report.valid_user_id(Domain::Project, &source_id, "user_id", owner_raw) | ||
| else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject conflicting project ownership instead of trusting embedded state.
When project.user_id is non-empty, it silently overrides document.user_id. A document stored under one user can therefore migrate as another user’s project. Require both IDs to match, or record an ownership loss and skip.
As per coding guidelines, preserve tenant, user, agent, and project scope on migrated records.
Proposed fix
- let owner_raw = if project.user_id.is_empty() {
- document.user_id.as_str()
- } else {
- project.user_id.as_str()
- };
+ if !project.user_id.is_empty() && project.user_id != document.user_id {
+ report.record_loss(
+ Domain::Project,
+ source_id,
+ "user_id",
+ LossReason::Unparseable,
+ "project owner does not match the source document scope",
+ );
+ continue;
+ }
+ let owner_raw = document.user_id.as_str();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let owner_raw = if project.user_id.is_empty() { | |
| document.user_id.as_str() | |
| } else { | |
| project.user_id.as_str() | |
| }; | |
| let Some(owner_user_id) = | |
| report.valid_user_id(Domain::Project, &source_id, "user_id", owner_raw) | |
| else { | |
| continue; | |
| }; | |
| if !project.user_id.is_empty() && project.user_id != document.user_id { | |
| report.record_loss( | |
| Domain::Project, | |
| source_id, | |
| "user_id", | |
| LossReason::Unparseable, | |
| "project owner does not match the source document scope", | |
| ); | |
| continue; | |
| } | |
| let owner_raw = document.user_id.as_str(); | |
| let Some(owner_user_id) = | |
| report.valid_user_id(Domain::Project, &source_id, "user_id", owner_raw) | |
| else { | |
| continue; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_migration/src/convert/projects.rs` around lines 47 -
56, Update the project ownership handling before calling report.valid_user_id in
the project conversion flow: when both project.user_id and document.user_id are
present, require them to match; on a mismatch, record the ownership loss through
the existing report mechanism and skip the project. Preserve the current
valid_user_id validation for non-conflicting ownership and ensure migrated
records retain the document’s tenant/user scope.
Source: Coding guidelines
|
Closing this because it still updates legacy top-level I opened #6323 to track the offline v1-to-Reborn migration workflow as Reborn-native work with implementation notes. |
This branch was successfully deployed
3 participants
Intent
Complete the safe offline v1-to-Reborn migration workflow for plan, apply, resume, verify, status, local-dev libSQL, production PostgreSQL, Docker and source-build companions. Preserve read-only sealed planning, target quarantine, deterministic replay, structural durable-store verification, explicit unsupported-data reporting, and all previously fixed CodeRabbit and review feedback. Address every remaining second-review issue from run 01KXDZDZ1VRJDXXWCACWXBGKS7: persist resumable Applying only after preflight or an atomic freshness claim; exclude only the exact nested Reborn target-owned tree from v1 home inventory; bind verified activation markers to the configured target fingerprint, scope and profile; add atomic run-bound libSQL target claims and durable parent-directory fsync; preserve or explicitly disposition source-agent memory scopes; reject divergent and deduplicate exact engine document duplicates; require absent-or-exact identity replay; pause routines missing next_fire_at; pause automations owned by suspended users; pause or disposition mission references to unmigrated projects; make missing source secret key material an unconditional blocker when secrets exist; make secret import atomic absent-or-exact; tolerate optional missing settings tables; classify heartbeat state truthfully as unsupported or operator action; validate PostgreSQL audit partition names exactly; stream PostgreSQL fingerprints without materializing all rows; report heartbeat loss only for actual heartbeat rows. Choose the safest fail-closed behavior for ambiguous policy cases. Revalidate the whole branch and update complete PR metadata. Live PostgreSQL and Docker end-to-end remain environmental evidence gaps unless available without external unsafe writes.
What Changed
ironclaw-reborn migrate v1plan, apply, resume, verify, and status workflow for libSQL and PostgreSQL, using sealed inventory manifests and durable-store verification.Risk Assessment
🚨 High: The default nested local workflow can poison its own source inventory, while marker races and identity/ownership gaps can leave successful targets quarantined or migrated records inconsistently scoped.
Testing
Automated backend and CLI suites, source-built companion pairs, and end-to-end libSQL plus isolated live PostgreSQL migrations succeeded after focused fixes, with durable reviewer-visible evidence; Docker’s static/entrypoint tests passed, but a live image run remains unverified because no permitted daemon was available.
Evidence: Local-dev libSQL migration lifecycle
Evidence: Production PostgreSQL migration lifecycle
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
crates/ironclaw_reborn_migration/src/main.rs:275- Apply persistsApplyingbefore source/manifest preflight. A crash or secret-input error in this window leaves an invalid plan quarantined as active, and the unconditional marker overwrite is not a freshness claim. Run preflight before persisting the transition or atomically claim the expected prior state.crates/ironclaw_reborn_migration/src/target.rs:109- Shared migration state is a no-op for libSQL, so concurrent runs can both observe an empty target and write mixed state; direct library callers also have no target-owned quarantine. Add a transactional, run-bound libSQL claim that accepts only absent or exact matching state.crates/ironclaw_reborn_cli/src/commands/migrate.rs:288- Activation accepts a marker using only schema, protocol, and status. Neither local nor PostgreSQL state is compared with the current target fingerprint, profile, tenant, or agent, so a staleverifiedmarker can unlock an unverified target after configuration changes. Persist and require an exact target/scope match.crates/ironclaw_reborn_migration/src/main.rs:511- The marker writer syncs the temporary file but not its parent directory after rename. A power loss can remove the only local quarantine marker after target writes became durable, allowing startup of a partial migration. Fsync the parent directory after marker and manifest installation/removal.crates/ironclaw_reborn_migration/src/target.rs:613- Trigger replay performsget_triggerfollowed by overwrite-capableupsert_trigger; a divergent record inserted between them is overwritten. Replace this with an atomic insert-if-absent followed by exact conflict reconciliation.crates/ironclaw_reborn_migration/src/target.rs:270- Verification inserts the configured tenant directly into SQLLIKEpatterns. Legal tenant IDs containing%or_broaden the match and can let other tenant paths satisfy verification counts. Escape wildcard characters and use an explicit SQL escape clause.crates/ironclaw_reborn_migration/src/inventory.rs:551- Home inventory excludes target files but still hashes/counts a newly created target directory and its known-directory ancestors. If the target is nested below a known v1 directory, resume sees migration-created metadata as source drift and cannot continue. Exclude the exact target-owned subtree and its target-only ancestor contribution while retaining siblings.crates/ironclaw_reborn_migration/src/source.rs:297- PostgreSQL inventory loads every JSON-encoded row withclient.queryand then copies all rows into anotherVec<String>. Large production/audit tables can exhaust migrator memory, and this scan runs repeatedly during plan and preflight. Stream rows into the digest and count incrementally.crates/ironclaw_reborn_migration/src/convert/memory.rs:62- All source memory documents, including unscoped and arbitrary source-agent documents, are flattened into the configured target agent. This silently changes ownership; duplicate paths either abort or inflate expected counts. Decide whether source agent scopes should be preserved or explicitly dispositioned, then keep each representable scope distinct.crates/ironclaw_reborn_migration/src/convert/memory.rs:86- Replay treats matching content with missing/different metadata as irreconcilable, but the native backend commits content and metadata separately. If metadata persistence fails after content commits, every resume fails permanently. Make the pair atomic or safely complete a recognized partial migration write.crates/ironclaw_memory_native/src/backend.rs:452- The new metadata read checks onlyfile_documents, ignoring the backend's explicitmetadatacapability. A backend configured with metadata disabled can therefore execute an unsupported operation. Fail closed oncapabilities.metadatabefore repository access.crates/ironclaw_reborn_migration/src/convert/automations.rs:124- An enabled routine with no durablenext_fire_atremains scheduled and falls back tocreated_at, commonly making it immediately overdue after cutover. Pause these routines and record the required operator action, matching the mission behavior.crates/ironclaw_reborn_migration/src/convert/automations.rs:124- Routine and mission trigger state is derived only from the automation record; it never checks the migrated owner's lifecycle. Automations belonging to suspended, deactivated, or synthesized-suspended users can therefore execute after cutover. Force their triggers toPaused.crates/ironclaw_reborn_migration/src/convert/automations.rs:401- Mission triggers and threads retainproject_idwithout confirming that the project converter imported that project. Unparseable or missing project documents can leave scheduled work scoped to a nonexistent project. Track imported project IDs and pause/disposition unresolved references.crates/ironclaw_reborn_migration/src/convert/automations.rs:283- Engine thread documents are inserted into a map with unconditional replacement, while mission duplicates are processed repeatedly. Divergent duplicate documents across layouts/scopes are silently selected by iteration order and exact duplicates inflate counts. Deduplicate exact representations and reject divergent records by durable ID.crates/ironclaw_reborn_migration/src/convert/secrets.rs:30- When source key material is absent, the converter reports one loss and returns success even if source secrets exist, allowing the run to reach Applied/Verified with every secret omitted. Make a missing source key an apply blocker whenever the inventory contains secret rows.crates/ironclaw_reborn_migration/src/convert/secrets.rs:181- Secret import checks metadata/material and then calls replacement-capableput. A concurrent value inserted after the check can be overwritten, violating absent-or-exact replay. Add an atomic insert-if-absent/exact-match import operation.crates/ironclaw_reborn_migration/src/inventory.rs:372- Audit partition recognition checks only suffix length and themposition, so malformed names such assecret_usage_log_yABCDmZZbypass the unknown-table blocker. Require the exactyYYYYmMMdigit shape and a valid month.crates/ironclaw_reborn_migration/src/inventory.rs:90- Inventory labelsheartbeat_stateas semantically converted even though the converter writes no target state, and the converter reports a loss for every discovered user rather than actual heartbeat rows. Classify nonzero heartbeat data as unsupported/operator action and enumerate only real heartbeat owners.crates/ironclaw_reborn_migration/src/convert/settings.rs:25- Optional historical schemas without asettingstable pass discovery but fail here becauseget_all_settingspropagates the missing-table error. Treat only an absent settings table as empty while preserving all other read failures.🔧 Fix: Harden Reborn migration safety and deterministic replay
10 issues (6 errors, 4 warnings) still open:
crates/ironclaw_reborn_migration/src/inventory.rs:408- Home inventory excludes the target database files and key, but not other Reborn-owned control files. With default paths, the v1 home is~/.ironclawand Reborn lives under~/.ironclaw/reborn; config, manifest, and migration-state files therefore become unknown source content or source drift, preventing local apply. Exclude the exact Reborn-owned subtree/control paths without hiding v1 siblings.crates/ironclaw_reborn_migration/src/inventory.rs:650- Excluded target-only ancestor directories are still added to counts and checksums when the containing known directory has a legitimate sibling. Creating a nested target under such a directory changes the sealed inventory and blocks resume. Skip recursively empty excluded-only children before hashing or counting them.crates/ironclaw_reborn_migration/src/main.rs:287- The localApplyingmarker is installed before the atomic target-owned run claim. Concurrent plans can both pass preflight, and the losing run can later overwrite the winner's local state with its ownPlannedorFailedmarker while durable state remains bound to the winner, permanently quarantining a successful migration. Claim first and condition local updates on the claimed run.crates/ironclaw_reborn_migration/src/main.rs:293- When the duplicated inner preflight fails, this branch writes a localPlannedmarker even though no durable claim exists. The activation guard rejects all local-only markers, leaving an otherwise untouched target quarantined after a transient preflight failure. Durably remove/restore the prior marker instead.crates/ironclaw_reborn_migration/src/convert/secrets.rs:36- If source secrets exist but the target secret store is unavailable, the converter records one loss and succeeds. Applied checkpoints then expect zero secrets, allowing verification while every source secret was omitted. Treat missing target encryption/store capability as an apply blocker whenever secret rows exist.crates/ironclaw_reborn_migration/src/convert/projects.rs:96- Duplicate project IDs are compared only after deserializing through a subset DTO and normalizing toProjectRecord. Source documents differing in ignored engine fields or missing-versus-default representation are accepted as exact duplicates, silently discarding divergent state. Retain and compare the raw parsed representation as well.crates/ironclaw_reborn_migration/src/convert/projects.rs:47- An engine project's embeddeduser_idoverrides the memory-document owner, but synthesized-user discovery sees only table-level owners. An embedded owner with no other source row is never created, so projects and mission threads can reference a nonexistent Reborn user. Decide which owner is authoritative, then reject/report mismatches or synthesize embedded owners before importing references.crates/ironclaw_reborn_migration/src/convert/users.rs:44- API-token reauthentication losses are enumerated only for canonicalusersrows. On older schemas without that table, or for orphan token owners, the synthesized user is imported but its token hashes disappear from the per-record apply report. Enumerate tokens for every discovered owner and deduplicate canonical owners.crates/ironclaw_reborn_migration/src/target.rs:1013- The migrator opens PostgreSQL with a separate legacy TLS policy rather than the composition-owned production policy. It ignoresDATABASE_SSLMODEand the explicit remote-cleartext setting; even localpreferbehavior differs, so a production-openable target may be unmigratable. Reuse the composition PostgreSQL pool builder and options.crates/ironclaw_reborn_cli/src/commands/doctor.rs:303- When durable target state is absent, doctor trusts only the rawstatusstring in the onboarding manifest. An edited or corrupted planned manifest containingverifiedtherefore reports a passing verification despite no durable verification state. Validate the sealed manifest and reserveverifiedfor matching target-owned state.cargo test -p ironclaw_reborn_migration --no-default-features --features libsql(initial run found stale lifecycle assertions; clean after test-only fixes)cargo test -p ironclaw_reborn_cli --no-default-features --features libsql --test migration_clicargo test -p ironclaw_reborn_cli --no-default-features --features libsql --test smoke dockercargo test -p ironclaw_reborn_cli --no-default-features --features libsql --bin ironclaw-reborn commands::migrate::testscargo test -p ironclaw_secrets filesystem_secret_store_atomic_insert_never_overwrites_divergent_materialcargo test -p ironclaw_reborn_identity import_migrated_usercargo test -p ironclaw_triggers --features libsql --test repository_contract libsql_insert_if_absent_never_overwritescargo test -p ironclaw_reborn_migration --no-default-features --features postgres(feature-path tests; not live PostgreSQL E2E)Built the libSQL companion, seeded a v1 database with a user and no optionalsettingstable, then ran the realironclaw-reborn migrate v1 plan,apply,resume,verify, andstatus --jsoncommandsChecked target absence after planning withtest ! -e "$TARGET", compared source snapshots withshasum -a 256anddiff -u, and queriedroot_filesystem_entriesusingsqlite3 -jsonRead-only environment checks:docker version,docker context show,docker ps,pg_isready, and a read-onlypsql ... SELECT current_user, current_database(), current_setting('server_version')querygit diff --checkand final worktree review; removed package build outputs withcargo clean -p ironclaw_reborn_cli -p ironclaw_reborn_migration🔧 Fix: Align migration tests with run-bound lifecycle
1 warning still open:
Dockerfile.reborn- Live Docker image build/runtime evidence remains unavailable. Dockerfile and entrypoint smoke coverage passed, but the Docker daemon is not running; starting Docker Desktop or building through another external daemon would violate this run’s system-state boundary. Run the image build and migration entrypoint in an authorized Docker environment if live container evidence is required.bash scripts/codebase-graph.sh status(graph unavailable; used documented guidance/targeted search fallback)cargo test -p ironclaw_reborn_migration --no-default-features --features libsqlcargo test -p ironclaw_reborn_migration --no-default-features --features postgrescargo test -p ironclaw_reborn_cli(initial failures diagnosed and fixed; focused retry passed)cargo test -p ironclaw_reborn_cli --features libsqlcargo test -p ironclaw_reborn_cli --features postgres --test smokecargo build -p ironclaw_reborn_cli --features libsql --bin ironclaw-rebornplus matching libSQL migration companion buildPrimary CLIplan → apply → resume → verify → status --jsonagainst an isolated v1 libSQL snapshot, followed by durable SQLite state/user readbackcargo build -p ironclaw_reborn_cli --features postgres --bin ironclaw-rebornplus matching PostgreSQL migration companion buildInitialized PostgreSQL 18 under the evidence directory, applied the v1 schema, ran primary CLIplan → apply → resume → verify → status --json, queried durable shared state/user rows, then stopped PostgreSQLDockerfile/entrypoint behavior exercised by the CLI smoke suitesdocker info --format '{{.ServerVersion}}'(daemon unavailable)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.