feat(reborn): scoped-lifecycle admin install store (#3288) — availability foundation for #5261#4544
feat(reborn): scoped-lifecycle admin install store (#3288) — availability foundation for #5261#4544serrrfirat wants to merge 14 commits into
Conversation
github-actions
Bot
added
scope: docs
![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 package-kind agnostic scoped lifecycle ownership model and store foundation for Reborn package administration, allowing admin-shared and user-private package resolution. It adds the core domain models, resolution logic, and durable filesystem-based storage adapters with LibSql and Postgres backends, along with a comprehensive V1 feature parity audit. The code review feedback suggests several improvements to enhance robustness and correctness: resolving inconsistent candidate ordering in the effective installation resolution logic, adding defensive checks to verify installation IDs during retrieval, ensuring the immutability of installation IDs and preventing out-of-order updates during validation, and replacing a hand-rolled hex encoding utility with the standard hex crate.
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.
| pub fn resolve_effective_scoped_lifecycle_installations( | ||
| subject: ScopedLifecycleSubject, | ||
| candidates: impl IntoIterator<Item = ScopedLifecycleInstallation>, | ||
| ) -> EffectiveScopedLifecycleInstallations { | ||
| let mut effective = BTreeMap::<String, ScopedLifecycleInstallation>::new(); | ||
|
|
||
| for installation in candidates { | ||
| if !installation.enabled || !installation_visible_to(&installation, &subject) { | ||
| continue; | ||
| } | ||
| let key = package_key(&installation.package_ref); | ||
| match installation.ownership { | ||
| ScopedLifecycleOwnership::AdminShared { .. } => { | ||
| effective.entry(key).or_insert(installation); | ||
| } | ||
| ScopedLifecycleOwnership::UserPrivate { .. } => { | ||
| effective.insert(key, installation); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| EffectiveScopedLifecycleInstallations { | ||
| subject, | ||
| installations: effective.into_values().collect(), | ||
| } | ||
| } |
There was a problem hiding this comment.
The current resolution logic in resolve_effective_scoped_lifecycle_installations handles same-ownership overrides inconsistently depending on the order of candidates. Specifically, for AdminShared installations, the first candidate wins (due to or_insert), whereas for UserPrivate installations, the last candidate wins (due to insert).
To ensure deterministic and consistent behavior regardless of candidate ordering, we should explicitly define the precedence rules and use a consistent tie-breaker (such as the updated_at timestamp or lexicographical order of installation_id) when resolving same-ownership installations.
pub fn resolve_effective_scoped_lifecycle_installations(
subject: ScopedLifecycleSubject,
candidates: impl IntoIterator<Item = ScopedLifecycleInstallation>,
) -> EffectiveScopedLifecycleInstallations {
let mut effective = BTreeMap::<String, ScopedLifecycleInstallation>::new();
for installation in candidates {
if !installation.enabled || !installation_visible_to(&installation, &subject) {
continue;
}
let key = package_key(&installation.package_ref);
match effective.get(&key) {
Some(existing) => {
let should_replace = match (&existing.ownership, &installation.ownership) {
(ScopedLifecycleOwnership::AdminShared { .. }, ScopedLifecycleOwnership::UserPrivate { .. }) => true,
(ScopedLifecycleOwnership::UserPrivate { .. }, ScopedLifecycleOwnership::AdminShared { .. }) => false,
_ => installation.updated_at > existing.updated_at
|| (installation.updated_at == existing.updated_at && installation.installation_id < existing.installation_id),
};
if should_replace {
effective.insert(key, installation);
}
}
None => {
effective.insert(key, installation);
}
}
}
EffectiveScopedLifecycleInstallations {
subject,
installations: effective.into_values().collect(),
}
}| async fn get_installation( | ||
| &self, | ||
| tenant_id: &TenantId, | ||
| installation_id: &ScopedLifecycleInstallationId, | ||
| ) -> Result<Option<ScopedLifecycleInstallation>, ProductWorkflowError> { | ||
| let path = scoped_lifecycle_installation_path(&self.root, tenant_id, installation_id)?; | ||
| let Some(entry) = self | ||
| .filesystem | ||
| .get(&path) | ||
| .await | ||
| .map_err(|error| scoped_lifecycle_filesystem_error("load installation", error))? | ||
| else { | ||
| return Ok(None); | ||
| }; | ||
| let installation = parse_scoped_lifecycle_installation(entry)?; | ||
| if installation.tenant_id() != tenant_id { | ||
| return Err(scoped_lifecycle_transient( | ||
| "scoped lifecycle installation tenant mismatch", | ||
| )); | ||
| } | ||
| Ok(Some(installation)) | ||
| } |
There was a problem hiding this comment.
In get_installation, we verify that the loaded installation's tenant_id matches the requested tenant_id, but we do not verify that the installation_id matches. Adding a defensive check to ensure the loaded installation_id matches the requested one prevents potential data corruption or path mismatch issues.
async fn get_installation(
&self,
tenant_id: &TenantId,
installation_id: &ScopedLifecycleInstallationId,
) -> Result<Option<ScopedLifecycleInstallation>, ProductWorkflowError> {
let path = scoped_lifecycle_installation_path(&self.root, tenant_id, installation_id)?;
let Some(entry) = self
.filesystem
.get(&path)
.await
.map_err(|error| scoped_lifecycle_filesystem_error("load installation", error))?
else {
return Ok(None);
};
let installation = parse_scoped_lifecycle_installation(entry)?;
if installation.tenant_id() != tenant_id {
return Err(scoped_lifecycle_transient(
"scoped lifecycle installation tenant mismatch",
));
}
if &installation.installation_id != installation_id {
return Err(scoped_lifecycle_transient(
"scoped lifecycle installation id mismatch",
));
}
Ok(Some(installation))
}| async fn load_installation( | ||
| &self, | ||
| tenant_id: &TenantId, | ||
| installation_id: &ScopedLifecycleInstallationId, | ||
| ) -> Result<Option<VersionedScopedLifecycleInstallation>, ProductWorkflowError> { | ||
| let path = scoped_lifecycle_installation_path(&self.root, tenant_id, installation_id)?; | ||
| let Some(entry) = self | ||
| .filesystem | ||
| .get(&path) | ||
| .await | ||
| .map_err(|error| scoped_lifecycle_filesystem_error("load installation", error))? | ||
| else { | ||
| return Ok(None); | ||
| }; | ||
| let loaded = parse_versioned_scoped_lifecycle_installation(entry)?; | ||
| if loaded.installation.tenant_id() != tenant_id { | ||
| return Err(scoped_lifecycle_transient( | ||
| "scoped lifecycle installation tenant mismatch", | ||
| )); | ||
| } | ||
| Ok(Some(loaded)) | ||
| } |
There was a problem hiding this comment.
Similarly to get_installation, we should verify that the loaded installation_id matches the requested installation_id in load_installation to ensure data integrity.
async fn load_installation(
&self,
tenant_id: &TenantId,
installation_id: &ScopedLifecycleInstallationId,
) -> Result<Option<VersionedScopedLifecycleInstallation>, ProductWorkflowError> {
let path = scoped_lifecycle_installation_path(&self.root, tenant_id, installation_id)?;
let Some(entry) = self
.filesystem
.get(&path)
.await
.map_err(|error| scoped_lifecycle_filesystem_error("load installation", error))?
else {
return Ok(None);
};
let loaded = parse_versioned_scoped_lifecycle_installation(entry)?;
if loaded.installation.tenant_id() != tenant_id {
return Err(scoped_lifecycle_transient(
"scoped lifecycle installation tenant mismatch",
));
}
if &loaded.installation.installation_id != installation_id {
return Err(scoped_lifecycle_transient(
"scoped lifecycle installation id mismatch",
));
}
Ok(Some(loaded))
}| fn validate_scoped_lifecycle_update_identity( | ||
| existing: &ScopedLifecycleInstallation, | ||
| next: &ScopedLifecycleInstallation, | ||
| ) -> Result<(), ProductWorkflowError> { | ||
| if existing.package_ref != next.package_ref { | ||
| return Err(scoped_lifecycle_invalid_request( | ||
| "package_ref cannot change for scoped lifecycle installation update", | ||
| )); | ||
| } | ||
| if existing.ownership != next.ownership { | ||
| return Err(scoped_lifecycle_invalid_request( | ||
| "ownership cannot change for scoped lifecycle installation update", | ||
| )); | ||
| } | ||
| if existing.created_by != next.created_by { | ||
| return Err(scoped_lifecycle_invalid_request( | ||
| "created_by cannot change for scoped lifecycle installation update", | ||
| )); | ||
| } | ||
| if existing.created_at != next.created_at { | ||
| return Err(scoped_lifecycle_invalid_request( | ||
| "created_at cannot change for scoped lifecycle installation update", | ||
| )); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
In validate_scoped_lifecycle_update_identity, we should explicitly verify that installation_id cannot change during an update. Additionally, we should check that next.updated_at is not less than existing.updated_at to prevent out-of-order updates or clock skew issues from overwriting newer data with older state.
fn validate_scoped_lifecycle_update_identity(
existing: &ScopedLifecycleInstallation,
next: &ScopedLifecycleInstallation,
) -> Result<(), ProductWorkflowError> {
if existing.installation_id != next.installation_id {
return Err(scoped_lifecycle_invalid_request(
"installation_id cannot change for scoped lifecycle installation update",
));
}
if existing.package_ref != next.package_ref {
return Err(scoped_lifecycle_invalid_request(
"package_ref cannot change for scoped lifecycle installation update",
));
}
if existing.ownership != next.ownership {
return Err(scoped_lifecycle_invalid_request(
"ownership cannot change for scoped lifecycle installation update",
));
}
if existing.created_by != next.created_by {
return Err(scoped_lifecycle_invalid_request(
"created_by cannot change for scoped lifecycle installation update",
));
}
if existing.created_at != next.created_at {
return Err(scoped_lifecycle_invalid_request(
"created_at cannot change for scoped lifecycle installation update",
));
}
if next.updated_at < existing.updated_at {
return Err(scoped_lifecycle_invalid_request(
"updated_at cannot be decreased for scoped lifecycle installation update",
));
}
Ok(())
}| fn hex_component(value: &str) -> String { | ||
| const HEX: &[u8; 16] = b"0123456789abcdef"; | ||
| let mut encoded = String::with_capacity(value.len() * 2); | ||
| for byte in value.as_bytes() { | ||
| encoded.push(HEX[(byte >> 4) as usize] as char); | ||
| encoded.push(HEX[(byte & 0x0f) as usize] as char); | ||
| } | ||
| encoded | ||
| } |
There was a problem hiding this comment.
Avoid hand-rolling common utility functions such as hex encoding. Instead, use well-established external crates (like the hex crate) to ensure efficiency, correctness, and maintainability.
fn hex_component(value: &str) -> String {
hex::encode(value)
}References
- Avoid hand-rolling common utility functions such as hex encoding. Instead, use well-established external crates (like the
hexcrate) to ensure efficiency, correctness, and maintainability.
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: COMMENT (requires extended review time)
Note: This PR changes 3354 lines across 9 files, introducing scoped lifecycle admin foundation. A thorough review requires more time than available in this automated batch. Flagging for manual review.
Quick Surface Scan
- Size: 3354 lines changed
- Files: 9 (including 2 doc files, 5 Rust implementation files, 2 tests/AGENTS.md)
- Scope: Product workflow lifecycle management + storage layer
- Parity tracking: Updates FEATURE_PARITY.md and V1_REBORN_PARITY_AUDIT.md
Recommended Review Approach
Given the size and scope, suggest:
- Read
crates/ironclaw_product_workflow_storage/AGENTS.mdchanges first (context) - Review
scoped_lifecycle.rsmodule contracts (both crates) - Check test coverage in
durable_ledger_contract.rs - Verify FEATURE_PARITY.md updates match implementation
- Focus on:
- New storage operations (DB trait changes)
- Lifecycle state transitions (correctness)
- Error handling paths
- Test coverage for new admin operations
Quick Observations
Positive signals:
- Updates parity tracking docs (good discipline)
- Includes AGENTS.md documentation
- Has test file changes
Needs verification:
- DB changes support both PostgreSQL and libSQL (AGENTS.md requirement)
- New lifecycle operations have error-path tests
- No breaking API changes without migration path
- Scoped admin boundaries respect existing authorization model
Recommend: Schedule dedicated review session for this PR given complexity and risk level (workflow + storage changes).
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Review: #4544 — Add scoped lifecycle admin foundation for Reborn capabilities
Intent Analysis
Stated goal: Introduce admin-shared / user-private scoped lifecycle model and durable filesystem-backed store (libSQL + PostgreSQL) for multi-tenant Reborn package management. WebUI/API wiring and adapter activation are explicitly deferred.
Scope: ironclaw_product_workflow (model) + ironclaw_product_workflow_storage (store) + documentation.
Understanding confidence: High.
Reviewer Findings
🔴 Thermo-Nuclear — HIGH
crates/ironclaw_product_workflow_storage/src/scoped_lifecycle.rs is 1296 lines (new file)
The file is created at 1296 lines — well past the 1000-line threshold. It mixes:
FilesystemScopedLifecycleInstallationStorecore logic (~440 lines)- Thin DB-wrapper types:
RebornLibSql*/RebornPostgres*(~120 lines, boilerplate delegation) - Path computation helpers (~100 lines)
- Entry serialization helpers (~120 lines)
- State enums + helper types (~40 lines)
- Unit test module (~280 lines)
Suggested decomposition:
scoped_lifecycle/
mod.rs — public types re-exported, common helpers
store.rs — FilesystemScopedLifecycleInstallationStore + DB wrappers
paths.rs — path computation functions
entries.rs — Entry serialization/deserialization helpers
This makes each part independently navigable without a 1300-line scroll.
🟡 Tests — MEDIUM (confidence 75)
Missing: admin_shared rejects non-admin actor in domain model
ScopedLifecycleInstallation::admin_shared returns Err(BindingAccessDenied) when the actor role is not Admin. No test exercises this path in the domain-model test module. Test needed:
tests::admin_shared_rejects_non_admin_actor🟡 Tests — MEDIUM (confidence 65)
Missing: cross-tenant isolation in resolve_effective_scoped_lifecycle_installations
is_visible_to enforces tenant isolation, but no test verifies that an admin-shared installation from tenant-A is NOT visible to a subject in tenant-B. Given multi-tenancy is a core security property here, this gap should be closed.
tests::scoped_lifecycle::effective_resolution_excludes_other_tenant_installations🟢 Strengths
- Ownership model clean:
can_be_mutated_by+is_visible_tocorrectly enforce tenant + role boundaries. - CAS-based concurrency thorough: two-phase (ID reservation + package path) with tombstone-on-failure cleanup.
- Concurrent race contract tests (
tokio::join!with two store handles) are excellent. - Immutable identity enforcement (
package_ref,ownership,created_by,created_atcannot change on update) correctly implemented and tested. - Audit trail (
created_by/updated_bymatching actor) validated in both directions. - Tombstone semantics allow safe delete → recreate lifecycle without storage collision.
V1_REBORN_PARITY_AUDIT.mdis genuinely useful; the API-first direction is the right call.
APPROVE Bar Audit
| Condition | Result |
|---|---|
| No security finding ≥50 confidence | ✅ PASS |
| No bug finding ≥60 confidence | ✅ PASS |
| No file pushed past 1000 lines | ❌ FAIL — scoped_lifecycle.rs storage: 1296 lines (new file) |
| No hardcoded strings duplicating production | ✅ PASS |
| No silent wildcard drops | ✅ PASS |
| No breaking API change without migration | ✅ PASS — purely additive |
| No reachable failure path missing tests ≥65 | ❌ FAIL — admin_shared non-admin rejection path untested (confidence 75) |
| No thermo High findings | ❌ FAIL — 1296-line new file |
Verdict: COMMENT — 3 conditions fail. Core logic and design are solid. Blocking items: (1) decompose scoped_lifecycle.rs storage into submodules, (2) add the two test gaps noted above.
| const SCOPED_LIFECYCLE_ID_TOMBSTONE_RECORD_KIND: &str = | ||
| "scoped_lifecycle_installation_id_tombstone"; | ||
|
|
||
| pub struct FilesystemScopedLifecycleInstallationStore { |
There was a problem hiding this comment.
🔴 Thermo High — file too large. This new file is 1296 lines. The 1000-line threshold is a hard code-quality gate here. Suggest decomposing into scoped_lifecycle/store.rs (core store + DB wrappers), scoped_lifecycle/paths.rs (path computation), and scoped_lifecycle/entries.rs (Entry serialization). The test module alone is ~280 lines and could live in store.rs or a sibling tests.rs.
| } | ||
|
|
||
| impl ScopedLifecycleInstallation { | ||
| pub fn admin_shared( |
There was a problem hiding this comment.
🟡 Missing test: admin_shared rejects non-admin actor. admin_shared returns Err(BindingAccessDenied) when actor.role != Admin, but no test in this module covers that error path. Add:
#[test]
fn admin_shared_rejects_non_admin_actor() {
let t = tenant("t");
let user = ScopedLifecycleActor::user(t, user("u"));
assert_eq!(
ScopedLifecycleInstallation::admin_shared(install_id("x"), package("pkg"), user, Utc::now()),
Err(ProductWorkflowError::BindingAccessDenied)
);
}| pub installations: Vec<ScopedLifecycleInstallation>, | ||
| } | ||
|
|
||
| pub fn resolve_effective_scoped_lifecycle_installations( |
There was a problem hiding this comment.
🟡 Missing test: cross-tenant isolation. is_visible_to guards tenant boundaries but no test verifies that admin-shared from tenant-A is invisible to a subject in tenant-B. Multi-tenancy is a core invariant; add:
#[test]
fn effective_resolution_excludes_other_tenant_installations() {
let tenant_a = tenant("tenant-a");
let tenant_b = tenant("tenant-b");
let admin_a = ScopedLifecycleActor::admin(tenant_a.clone(), user("admin-a"));
let subject_b = ScopedLifecycleSubject::new(tenant_b, user("user-b"));
let shared_a = ScopedLifecycleInstallation::admin_shared(
install_id("shared-github"), package("github"), admin_a, Utc::now()
).expect("ok");
let effective = resolve_effective_scoped_lifecycle_installations(subject_b, [shared_a]);
assert!(effective.installations.is_empty());
}…ifecycle-admin # Conflicts: # Cargo.lock # FEATURE_PARITY.md # crates/ironclaw_product_workflow/src/lib.rs # crates/ironclaw_product_workflow_storage/Cargo.toml # crates/ironclaw_product_workflow_storage/src/lib.rs # crates/ironclaw_product_workflow_storage/tests/durable_ledger_contract.rs
Rebase #4544 onto current main (106 commits of drift). Resolved 2 conflicts: product_workflow/src/lib.rs export list (kept both LifecycleSearchExtensionSummary from main and lifecycle_package_kind_label from #4544); FEATURE_PARITY.md (kept #4544's scoped-lifecycle clause on Hosted MCP, main's newer #5256 rows for NEAR AI MCP and Tool policies). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds scoped lifecycle ownership and resolution types, wires them through storage and runtime policy selection, expands contract coverage, and updates parity/audit documentation. ChangesScoped lifecycle workflow, storage, and policy wiring
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Suggested reviewers
Possibly related issues
Poem
🚥 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 |
|
Rebased onto current
Non-destructive: a fast-forward merge of Taking this over to land as the scoped-lifecycle foundation for the capability-policy epic #5261 (continues #4628). The WebUI/API wiring and adapter activation called out as pending here are the follow-up PRs in that epic, built on top of this store. @serrrfirat heads-up 👆 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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_product_workflow_storage/src/scoped_lifecycle.rs`:
- Around line 78-89: The scoped lifecycle error logger in
scoped_lifecycle_durable_error only records the error type, so concrete
CAS/path/codec failures are hidden. Update this helper to log the actual error
value from the error parameter in the tracing::error! call, and keep the
operation context so failures are distinct and loud. Use the existing
scoped_lifecycle_durable_error and scoped_lifecycle_transient flow, and make
sure the mapped ProductWorkflowError still carries a human-readable reason
derived from the concrete error.
In `@crates/ironclaw_product_workflow_storage/src/scoped_lifecycle/store.rs`:
- Around line 243-275: The delete path can tombstone an installation-id
reservation that has already been reused by a recreate, leaving the package
record live but its mapping gone. Update the tombstoning logic in
tombstone_installation_id_reservation_if_current to re-check the current package
state before writing the tombstone, and skip tombstoning if the reservation is
no longer current for that path. Use reserve_installation_id and the
installation_id_state/ScopedLifecycleInstallationIdReservation flow as the
reference points, and add a regression test that races delete versus recreate
with the same installation id to verify list_installations, get_installation,
and subsequent deletes stay consistent.
In
`@crates/ironclaw_product_workflow_storage/src/scoped_lifecycle/store/tests.rs`:
- Around line 97-118: The filesystem mock in the scoped_lifecycle store tests is
ignoring the requested VirtualPath, which lets reservation and package lookups
blur together. Update the mock behavior in get() and query() so they only return
entries that match the requested path and the data seeded via with_entries(),
instead of falling back to self.entry for any miss. Keep the distinction between
the reservation path and package path intact so callers like
parse_installation_id_reservation() and the delete/update flows exercise the
real lookup path.
In `@crates/ironclaw_product_workflow/src/scoped_lifecycle.rs`:
- Around line 248-253: The delete request currently accepts a caller-supplied
tenant_id even though ScopedLifecycleActor already defines the only tenant this
operation should use. Update DeleteScopedLifecycleInstallationRequest handling
in scoped_lifecycle so the delete path derives tenant scope from
request.actor.tenant_id in the store, or at minimum validates request.tenant_id
matches request.actor.tenant_id before any storage IO. Make sure the
scoped_lifecycle::store delete flow fails closed and never builds or tombstones
a reservation path using a mismatched tenant_id.
- Around line 21-66: ScopedLifecycleInstallationId should follow the canonical
validated-newtype pattern instead of custom serde impls. Update the type to use
a shared validate(&str) helper plus #[serde(try_from = "String")] (and the
corresponding TryFrom path) so serialization/deserialization relies on the
derived serde flow. Keep the existing accessors on
ScopedLifecycleInstallationId, but align them with the validated-newtype
convention by providing as_ref()/as_str()/into_inner() and removing the
hand-written Serialize and Deserialize implementations.
In `@FEATURE_PARITY.md`:
- Line 340: Clarify the Hosted MCP extensions parity row so it does not imply
shipped activation behavior when only the storage and resolution foundation
exists. Update the wording in FEATURE_PARITY.md to separate the product-workflow
lifecycle model/store foundation and package resolution support from the
still-pending WebUI/API wiring and adapter activation path, using the Hosted MCP
extensions entry as the anchor. Keep the sentence aligned with the
storage-boundary guidance from the product workflow storage docs so “can
activate” is replaced with language that only describes what is actually
implemented today.
In `@V1_REBORN_PARITY_AUDIT.md`:
- Line 3: The audit header date is inconsistent with the later issue-coverage
timestamp, so update the top-level date in V1_REBORN_PARITY_AUDIT.md to match
the actual audit timeline or clearly label the 2026-06-08 coverage note as a
follow-up update. Keep the dates in the document aligned so the header and the
issue-coverage section do not contradict each other.
🪄 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: b6f51e93-b2a0-41fa-9ca9-bd51ef910764
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (14)
FEATURE_PARITY.mdV1_REBORN_PARITY_AUDIT.mdcrates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/src/lifecycle.rscrates/ironclaw_product_workflow/src/scoped_lifecycle.rscrates/ironclaw_product_workflow_storage/AGENTS.mdcrates/ironclaw_product_workflow_storage/Cargo.tomlcrates/ironclaw_product_workflow_storage/src/lib.rscrates/ironclaw_product_workflow_storage/src/scoped_lifecycle.rscrates/ironclaw_product_workflow_storage/src/scoped_lifecycle/entries.rscrates/ironclaw_product_workflow_storage/src/scoped_lifecycle/paths.rscrates/ironclaw_product_workflow_storage/src/scoped_lifecycle/store.rscrates/ironclaw_product_workflow_storage/src/scoped_lifecycle/store/tests.rscrates/ironclaw_product_workflow_storage/tests/durable_ledger_contract.rs
| fn scoped_lifecycle_durable_error( | ||
| operation: &'static str, | ||
| error: impl std::fmt::Display, | ||
| ) -> ProductWorkflowError { | ||
| let error_type = std::any::type_name_of_val(&error); | ||
| tracing::error!( | ||
| operation, | ||
| error_type, | ||
| "product workflow scoped lifecycle store failed" | ||
| ); | ||
| scoped_lifecycle_transient(format!("scoped lifecycle store failed to {operation}")) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Log the concrete storage error, not just its type.
Line 83 only records error_type, so CAS/path/codec failures collapse into the same log line even though this layer is supposed to fail loud.
Suggested fix
fn scoped_lifecycle_durable_error(
operation: &'static str,
error: impl std::fmt::Display,
) -> ProductWorkflowError {
let error_type = std::any::type_name_of_val(&error);
tracing::error!(
operation,
error_type,
+ error = %error,
"product workflow scoped lifecycle store failed"
);
scoped_lifecycle_transient(format!("scoped lifecycle store failed to {operation}"))
}As per path instructions, "Fail loud: flag silent-failure patterns". As per coding guidelines, "Map errors with context using .map_err(|e| SomeError::Variant { reason: e.to_string() })?".
📝 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.
| fn scoped_lifecycle_durable_error( | |
| operation: &'static str, | |
| error: impl std::fmt::Display, | |
| ) -> ProductWorkflowError { | |
| let error_type = std::any::type_name_of_val(&error); | |
| tracing::error!( | |
| operation, | |
| error_type, | |
| "product workflow scoped lifecycle store failed" | |
| ); | |
| scoped_lifecycle_transient(format!("scoped lifecycle store failed to {operation}")) | |
| } | |
| fn scoped_lifecycle_durable_error( | |
| operation: &'static str, | |
| error: impl std::fmt::Display, | |
| ) -> ProductWorkflowError { | |
| let error_type = std::any::type_name_of_val(&error); | |
| tracing::error!( | |
| operation, | |
| error_type, | |
| error = %error, | |
| "product workflow scoped lifecycle store failed" | |
| ); | |
| scoped_lifecycle_transient(format!("scoped lifecycle store failed to {operation}")) | |
| } |
🤖 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_product_workflow_storage/src/scoped_lifecycle.rs` around
lines 78 - 89, The scoped lifecycle error logger in
scoped_lifecycle_durable_error only records the error type, so concrete
CAS/path/codec failures are hidden. Update this helper to log the actual error
value from the error parameter in the tracing::error! call, and keep the
operation context so failures are distinct and loud. Use the existing
scoped_lifecycle_durable_error and scoped_lifecycle_transient flow, and make
sure the mapped ProductWorkflowError still carries a human-readable reason
derived from the concrete error.
Sources: Coding guidelines, Path instructions
| async fn reserve_installation_id( | ||
| &self, | ||
| path: &VirtualPath, | ||
| installation: &ScopedLifecycleInstallation, | ||
| ) -> Result<Option<RecordVersion>, ProductWorkflowError> { | ||
| let reservation = ScopedLifecycleInstallationIdReservation::new(installation); | ||
| let cas = match self.installation_id_state(path).await? { | ||
| InstallationIdState::Absent => CasExpectation::Absent, | ||
| InstallationIdState::Tombstone(version) => CasExpectation::Version(version), | ||
| InstallationIdState::Reserved(existing, _) => { | ||
| if existing.matches_installation(installation) { | ||
| return Ok(None); | ||
| } | ||
| return Err(scoped_lifecycle_invalid_request( | ||
| "scoped lifecycle installation id already exists", | ||
| )); | ||
| } | ||
| }; | ||
| self.filesystem | ||
| .put( | ||
| path, | ||
| entry_for_installation_id_reservation(&reservation)?, | ||
| cas, | ||
| ) | ||
| .await | ||
| .map(Some) | ||
| .map_err(|error| match error { | ||
| FilesystemError::VersionMismatch { .. } => scoped_lifecycle_invalid_request( | ||
| "scoped lifecycle installation id already exists", | ||
| ), | ||
| error => scoped_lifecycle_filesystem_error("reserve installation id", error), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Prevent delete from tombstoning a reservation that has just been reused.
reserve_installation_id() treats a matching live reservation as reusable, but tombstone_installation_id_reservation_if_current() later tombstones that same reservation without re-checking whether the package path has already been recreated. The interleaving delete(package tombstone) -> recreate(reuse reservation) -> delete(clean reservation) leaves a live package record with a tombstoned installation-id mapping, so list_installations() still returns it while get_installation()/subsequent deletes resolve None.
Suggested fix
async fn tombstone_installation_id_reservation_if_current(
&self,
path: &VirtualPath,
installation: &ScopedLifecycleInstallation,
) -> Result<(), ProductWorkflowError> {
let InstallationIdState::Reserved(reservation, version) =
self.installation_id_state(path).await?
else {
return Ok(());
};
if !reservation.matches_installation(installation) {
return Ok(());
}
+ let package_path =
+ scoped_lifecycle_installation_path_for_reservation(&self.root, &reservation)?;
+ if matches!(self.package_path_state(&package_path).await?, PackagePathState::Occupied) {
+ return Ok(());
+ }
self.tombstone_installation_id_reservation(path, &reservation, version)
.await?;
Ok(())
}Please add a regression test that races delete vs recreate with the same installation id.
Also applies to: 293-307
🤖 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_product_workflow_storage/src/scoped_lifecycle/store.rs`
around lines 243 - 275, The delete path can tombstone an installation-id
reservation that has already been reused by a recreate, leaving the package
record live but its mapping gone. Update the tombstoning logic in
tombstone_installation_id_reservation_if_current to re-check the current package
state before writing the tombstone, and skip tombstoning if the reservation is
no longer current for that path. Use reserve_installation_id and the
installation_id_state/ScopedLifecycleInstallationIdReservation flow as the
reference points, and add a regression test that races delete versus recreate
with the same installation id to verify list_installations, get_installation,
and subsequent deletes stay consistent.
| async fn get(&self, path: &VirtualPath) -> Result<Option<VersionedEntry>, FilesystemError> { | ||
| if let Some(entry) = self.entries.lock().await.get(path.as_str()).cloned() { | ||
| return Ok(Some(entry)); | ||
| } | ||
| Ok(self.entry.lock().await.clone()) | ||
| } | ||
|
|
||
| async fn list_dir(&self, path: &VirtualPath) -> Result<Vec<DirEntry>, FilesystemError> { | ||
| Err(FilesystemError::Unsupported { | ||
| path: path.clone(), | ||
| operation: FilesystemOperation::ListDir, | ||
| }) | ||
| } | ||
|
|
||
| async fn query( | ||
| &self, | ||
| _path: &VirtualPath, | ||
| _filter: &Filter, | ||
| _page: Page, | ||
| ) -> Result<Vec<VersionedEntry>, FilesystemError> { | ||
| Ok(self.entry.lock().await.iter().cloned().collect()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the filesystem mock honor the requested path.
get() returns self.entry for any path miss, and query() ignores with_entries() entirely. That means tests can pass without ever seeding the reservation path: for example, delete/update flows can read the package record when they asked for installation_ids/..., and parse_installation_id_reservation() still succeeds because those JSON fields overlap. This weakens the caller-level coverage the store needs.
Suggested fix
struct CapturingFilesystem {
- entry: Mutex<Option<VersionedEntry>>,
entries: Mutex<HashMap<String, VersionedEntry>>,
put_error: Mutex<Option<FilesystemError>>,
observed_cas: Mutex<Vec<CasExpectation>>,
delete_count: Mutex<usize>,
}
impl CapturingFilesystem {
fn new(entry: Option<VersionedEntry>) -> Self {
Self {
- entry: Mutex::new(entry),
- entries: Mutex::new(HashMap::new()),
+ entries: Mutex::new(
+ entry.into_iter()
+ .map(|entry| (entry.path.as_str().to_string(), entry))
+ .collect(),
+ ),
put_error: Mutex::new(None),
observed_cas: Mutex::new(Vec::new()),
delete_count: Mutex::new(0),
}
}
@@
async fn get(&self, path: &VirtualPath) -> Result<Option<VersionedEntry>, FilesystemError> {
- if let Some(entry) = self.entries.lock().await.get(path.as_str()).cloned() {
- return Ok(Some(entry));
- }
- Ok(self.entry.lock().await.clone())
+ Ok(self.entries.lock().await.get(path.as_str()).cloned())
}
@@
async fn query(
&self,
_path: &VirtualPath,
_filter: &Filter,
_page: Page,
) -> Result<Vec<VersionedEntry>, FilesystemError> {
- Ok(self.entry.lock().await.iter().cloned().collect())
+ Ok(self.entries.lock().await.values().cloned().collect())
}
}As per path instructions, "Test through the caller" requires the real reservation/package lookup path to be exercised, and this mock currently bypasses that distinction.
📝 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.
| async fn get(&self, path: &VirtualPath) -> Result<Option<VersionedEntry>, FilesystemError> { | |
| if let Some(entry) = self.entries.lock().await.get(path.as_str()).cloned() { | |
| return Ok(Some(entry)); | |
| } | |
| Ok(self.entry.lock().await.clone()) | |
| } | |
| async fn list_dir(&self, path: &VirtualPath) -> Result<Vec<DirEntry>, FilesystemError> { | |
| Err(FilesystemError::Unsupported { | |
| path: path.clone(), | |
| operation: FilesystemOperation::ListDir, | |
| }) | |
| } | |
| async fn query( | |
| &self, | |
| _path: &VirtualPath, | |
| _filter: &Filter, | |
| _page: Page, | |
| ) -> Result<Vec<VersionedEntry>, FilesystemError> { | |
| Ok(self.entry.lock().await.iter().cloned().collect()) | |
| } | |
| async fn get(&self, path: &VirtualPath) -> Result<Option<VersionedEntry>, FilesystemError> { | |
| Ok(self.entries.lock().await.get(path.as_str()).cloned()) | |
| } | |
| async fn list_dir(&self, path: &VirtualPath) -> Result<Vec<DirEntry>, FilesystemError> { | |
| Err(FilesystemError::Unsupported { | |
| path: path.clone(), | |
| operation: FilesystemOperation::ListDir, | |
| }) | |
| } | |
| async fn query( | |
| &self, | |
| _path: &VirtualPath, | |
| _filter: &Filter, | |
| _page: Page, | |
| ) -> Result<Vec<VersionedEntry>, FilesystemError> { | |
| Ok(self.entries.lock().await.values().cloned().collect()) | |
| } |
🤖 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_product_workflow_storage/src/scoped_lifecycle/store/tests.rs`
around lines 97 - 118, The filesystem mock in the scoped_lifecycle store tests
is ignoring the requested VirtualPath, which lets reservation and package
lookups blur together. Update the mock behavior in get() and query() so they
only return entries that match the requested path and the data seeded via
with_entries(), instead of falling back to self.entry for any miss. Keep the
distinction between the reservation path and package path intact so callers like
parse_installation_id_reservation() and the delete/update flows exercise the
real lookup path.
Source: Path instructions
| #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] | ||
| pub struct ScopedLifecycleInstallationId(String); | ||
|
|
||
| impl ScopedLifecycleInstallationId { | ||
| pub fn new(value: impl Into<String>) -> Result<Self, ProductWorkflowError> { | ||
| validate_lifecycle_string( | ||
| value.into(), | ||
| "scoped lifecycle installation id", | ||
| LIFECYCLE_ID_MAX_BYTES, | ||
| ) | ||
| .map(Self) | ||
| } | ||
|
|
||
| pub fn as_str(&self) -> &str { | ||
| &self.0 | ||
| } | ||
|
|
||
| pub fn into_inner(self) -> String { | ||
| self.0 | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for ScopedLifecycleInstallationId { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| f.write_str(self.as_str()) | ||
| } | ||
| } | ||
|
|
||
| impl Serialize for ScopedLifecycleInstallationId { | ||
| fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> | ||
| where | ||
| S: Serializer, | ||
| { | ||
| serializer.serialize_str(self.as_str()) | ||
| } | ||
| } | ||
|
|
||
| impl<'de> Deserialize<'de> for ScopedLifecycleInstallationId { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| let value = String::deserialize(deserializer)?; | ||
| Self::new(value).map_err(de::Error::custom) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' crates/ironclaw_product_workflow/src/scoped_lifecycle.rsRepository: nearai/ironclaw
Length of output: 6608
🏁 Script executed:
sed -n '1,220p' crates/ironclaw_product_workflow/src/scoped_lifecycle.rsRepository: nearai/ironclaw
Length of output: 6608
🏁 Script executed:
sed -n '1,220p' crates/ironclaw_product_workflow/src/scoped_lifecycle.rsRepository: nearai/ironclaw
Length of output: 6608
🏁 Script executed:
rg -n "serde\\(try_from = \"String\"\\)|fn validate\\(|fn as_ref\\(&self\\) -> &str|impl Into<String> for" crates -g '*.rs'Repository: nearai/ironclaw
Length of output: 11605
🏁 Script executed:
rg -n "pub struct .*\\(String\\);|pub struct .*\\(.*String.*\\)" crates -g '*.rs' | head -n 40Repository: nearai/ironclaw
Length of output: 3904
🏁 Script executed:
python3 - <<'PY'
import subprocess, textwrap, os, sys
cmds = [
r"rg -n 'serde\(try_from = \"String\"\)|fn validate\(|fn as_ref\(&self\) -> &str|impl Into<String> for' crates -g '*.rs'",
r"rg -n 'pub struct .*\\(String\\);|pub struct .*\\(.*String.*\\)' crates -g '*.rs' | head -n 40",
]
for c in cmds:
print(f"$ {c}")
p = subprocess.run(c, shell=True, text=True, capture_output=True)
print(p.stdout)
print(p.stderr, file=sys.stderr)
PYRepository: nearai/ironclaw
Length of output: 553
🏁 Script executed:
rg -n "serde\\(try_from = \"String\"\\)|impl<'de> Deserialize<'de> for .*String|impl Serialize for .*String" crates/ironclaw_product_workflow/src -g '*.rs'Repository: nearai/ironclaw
Length of output: 625
🏁 Script executed:
rg -n "as_ref\\(\\) -> &str|into_inner\\(self\\) -> String|validate_lifecycle_string\\(" crates/ironclaw_product_workflow/src -g '*.rs'Repository: nearai/ironclaw
Length of output: 1398
Use the canonical validated-newtype shape here
This diverges from the validated-newtype rule in CLAUDE.md: ScopedLifecycleInstallationId should use #[serde(try_from = "String")] with a shared validate(&str) and as_ref()/as_str()/into_inner(), instead of hand-written Serialize/Deserialize impls.
🤖 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_product_workflow/src/scoped_lifecycle.rs` around lines 21 -
66, ScopedLifecycleInstallationId should follow the canonical validated-newtype
pattern instead of custom serde impls. Update the type to use a shared
validate(&str) helper plus #[serde(try_from = "String")] (and the corresponding
TryFrom path) so serialization/deserialization relies on the derived serde flow.
Keep the existing accessors on ScopedLifecycleInstallationId, but align them
with the validated-newtype convention by providing
as_ref()/as_str()/into_inner() and removing the hand-written Serialize and
Deserialize implementations.
Source: Coding guidelines
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct DeleteScopedLifecycleInstallationRequest { | ||
| pub actor: ScopedLifecycleActor, | ||
| pub tenant_id: TenantId, | ||
| pub installation_id: ScopedLifecycleInstallationId, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed on tenant scope for deletes.
tenant_id is caller-supplied here even though actor already carries the only tenant this request is allowed to touch. The downstream delete path uses request.tenant_id to build the reservation path before any ownership check, and on a miss it tombstones that path and returns Ok(()) (crates/ironclaw_product_workflow_storage/src/scoped_lifecycle/store.rs:173-216). A mismatched request can therefore poison another tenant’s installation-id reservation.
Fail-closed shape
pub struct DeleteScopedLifecycleInstallationRequest {
pub actor: ScopedLifecycleActor,
- pub tenant_id: TenantId,
pub installation_id: ScopedLifecycleInstallationId,
}Then derive tenant scope from request.actor.tenant_id in the store, or at minimum reject request.tenant_id != request.actor.tenant_id before any storage IO.
As per coding guidelines, "Preserve tenant/user/agent/project/mission/thread scope on authority, state, memory, process, network, outbound, resource, and event records" and "Fail closed for auth...".
🤖 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_product_workflow/src/scoped_lifecycle.rs` around lines 248 -
253, The delete request currently accepts a caller-supplied tenant_id even
though ScopedLifecycleActor already defines the only tenant this operation
should use. Update DeleteScopedLifecycleInstallationRequest handling in
scoped_lifecycle so the delete path derives tenant scope from
request.actor.tenant_id in the store, or at minimum validates request.tenant_id
matches request.actor.tenant_id before any storage IO. Make sure the
scoped_lifecycle::store delete flow fails closed and never builds or tombstones
a reservation path using a mismatched tenant_id.
Source: Coding guidelines
| | Plugin tools | ✅ | ✅ | WASM tools | | ||
| | GSuite WASM tools | ✅ | 🚧 | Reborn bundles operation-level Google Drive/Docs/Sheets/Slides WASM packages with host-mediated HTTP egress, product-auth scoped bearer injection, and manifest-declared Google OAuth setup metadata; full live-recorded parity remains follow-up | | ||
| | Hosted MCP extensions | ✅ | 🚧 | Reborn composes host-mediated MCP runtime, bundles the current Notion MCP supported tool set, wires Notion ProductAuth OAuth exchange/refresh, can use Reborn ProductAuth DCR OAuth setup through the host callback origin, and can activate hosted MCP packages with live `tools/list` schema discovery through host-staged product-auth credentials | | ||
| | Hosted MCP extensions | ✅ | 🚧 | Reborn composes host-mediated MCP runtime, bundles the current Notion MCP supported tool set, wires Notion ProductAuth OAuth exchange/refresh, can use Reborn ProductAuth DCR OAuth setup through the host callback origin, can activate hosted MCP packages with live `tools/list` schema discovery through host-staged product-auth credentials, and now has a product-workflow scoped lifecycle model/store foundation for admin-shared plus user-private package resolution; WebUI/API wiring and adapter activation against that scoped store remain pending | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the hosted MCP status wording.
“Can activate” reads as shipped behavior, but the same sentence says WebUI/API wiring and adapter activation are still pending. Split the store/resolution foundation from the actual activation path so the parity row doesn’t overclaim.
♻️ Suggested wording tweak
- ...can activate hosted MCP packages with live `tools/list` schema discovery through host-staged product-auth credentials, and now has a product-workflow scoped lifecycle model/store foundation for admin-shared plus user-private package resolution; WebUI/API wiring and adapter activation against that scoped store remain pending
+ ...has a product-workflow scoped lifecycle model/store foundation for admin-shared plus user-private package resolution; WebUI/API wiring and adapter activation against that scoped store remain pendingBased on the storage-boundary note in crates/ironclaw_product_workflow_storage/AGENTS.md, the docs should separate the foundation from activation wiring.
📝 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.
| | Hosted MCP extensions | ✅ | 🚧 | Reborn composes host-mediated MCP runtime, bundles the current Notion MCP supported tool set, wires Notion ProductAuth OAuth exchange/refresh, can use Reborn ProductAuth DCR OAuth setup through the host callback origin, can activate hosted MCP packages with live `tools/list` schema discovery through host-staged product-auth credentials, and now has a product-workflow scoped lifecycle model/store foundation for admin-shared plus user-private package resolution; WebUI/API wiring and adapter activation against that scoped store remain pending | | |
| | Hosted MCP extensions | ✅ | 🚧 | Reborn composes host-mediated MCP runtime, bundles the current Notion MCP supported tool set, wires Notion ProductAuth OAuth exchange/refresh, can use Reborn ProductAuth DCR OAuth setup through the host callback origin, and now has a product-workflow scoped lifecycle model/store foundation for admin-shared plus user-private package resolution; WebUI/API wiring and adapter activation against that scoped store remain pending | |
🤖 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 `@FEATURE_PARITY.md` at line 340, Clarify the Hosted MCP extensions parity row
so it does not imply shipped activation behavior when only the storage and
resolution foundation exists. Update the wording in FEATURE_PARITY.md to
separate the product-workflow lifecycle model/store foundation and package
resolution support from the still-pending WebUI/API wiring and adapter
activation path, using the Hosted MCP extensions entry as the anchor. Keep the
sentence aligned with the storage-boundary guidance from the product workflow
storage docs so “can activate” is replaced with language that only describes
what is actually implemented today.
| @@ -0,0 +1,1159 @@ | |||
| # V1 IronClaw vs Reborn Feature Parity Audit | |||
|
|
|||
| Date: 2026-06-07 | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the audit date with the issue-coverage timestamp.
The header says 2026-06-07, but the issue-coverage section says the issue list was checked on 2026-06-08. That makes the audit timeline self-contradictory. Please bump the header date or mark the later section as a follow-up update.
♻️ Suggested fix
-Date: 2026-06-07
+Date: 2026-06-08📝 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.
| Date: 2026-06-07 | |
| Date: 2026-06-08 |
🤖 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 `@V1_REBORN_PARITY_AUDIT.md` at line 3, The audit header date is inconsistent
with the later issue-coverage timestamp, so update the top-level date in
V1_REBORN_PARITY_AUDIT.md to match the actual audit timeline or clearly label
the 2026-06-08 coverage note as a follow-up update. Keep the dates in the
document aligned so the header and the issue-coverage section do not contradict
each other.
`ScopedLifecyclePolicyCapabilitySurfaceResolver` derives the per-(tenant, user) capability allow-set from #4544's scoped-lifecycle installations (admin-shared -> all users in the tenant; user-private -> owner only; disabled excluded), mapping each installed package to its model-visible capability ids via a `PackageCapabilitySource` seeded from the first-party extension catalog (`visible_capability_ids`, manifest `Visibility::Model`). Fail-closed but graceful: the resolver never returns `Err` (which would abort the turn at host construction). No resolvable user, a user with no grants, or a store read failure (including a not-yet-created installation set) all deny every capability while the turn still runs. Wired into `build_reborn_runtime`'s local-dev branch behind a new `capability-policy` feature (compiles the resolver + the `ironclaw_product_workflow_storage` dep) and *activated* per-runtime by `IRONCLAW_REBORN_CAPABILITY_POLICY` (default off, mirroring the `HooksActivationConfig` master-flag-default-off pattern). With the feature absent or the flag off, local-dev keeps the historical `AllowAll` surface, so existing flows and all current tests are unchanged. Tests: 8 module unit tests (admin-shared/user-private visibility, disabled exclusion, no-user + store-failure graceful deny, package->cap mapping, real first-party catalog seeding, principal precedence). Full composition lib suite (852) green under `--features capability-policy`; clippy clean in default and `--all-features` states. Part of #5261. Depends on #4544. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…§17) Design §1-§14 holds; §17 captures what's built (epic #5261 child issues) and the corrections implementation forced: first-party integrations are WASM extensions not MCP (§7/§12 fixed); per-user availability is a CapabilityPolicyDelta (#4544 can_be_mutated_by forbids an admin writing UserPrivate for others, so #4544 is tenant-shared + per-user rides deltas); the resolver is feature+IRONCLAW_REBORN_CAPABILITY_POLICY gated (default off); the scoped-lifecycle store roots under the /tenants durable mount (the /engine default has no backend); #5272 reworked to REST-created users; approval has no admin path yet; Reborn admin gate is host_api::UserRole not src/ownership. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…5273) (#5288) The storage + resolution foundation for the configuration / identity / approval dimensions. Adds, in `ironclaw_capability_policy`: - `CapabilityPolicyDeltaStore` — durable store of per-(tenant, scope, capability) `CapabilityPolicyDelta` rows (the admin grants). Keyed with an explicit tenant (PolicyScope carries none) so deltas never leak across tenants; upsert/delete/`deltas_for`/`list_subject_deltas`. The admin REST surface (#5268) writes here; the resolver reads. Mirrors the #4544 store shape. - `InMemoryCapabilityPolicyDeltaStore` — in-memory backend for tests / local-dev (durable filesystem / libSQL backend is the follow-on). - `StoreBackedPolicyResolver` — implements the #5262 `PolicyResolver` port by folding the capability default (#5263 `CapabilityDefaultPolicySource`) with the subject's stored deltas via `resolve_effective_policy` into an `EffectivePolicy`. Resolution is live (reads the store every call). `deltas_for` pre-filters to the subject (tenant-wide row + the subject's own user row; project scope is dormant in v1). Availability in the resulting `EffectivePolicy` is the policy view (default + deltas); combining it with the installation view (#4544 / #5267) — plus injecting config, applying the identity ownership filter, and merging the approval admin layer — is the enforcement layer (the remaining #5273 work). Tests: 6 (upsert/read-back/delete, tenant-wide vs user-only visibility, cross- tenant isolation, default->tenant->user fold incl. config deep-merge + approval override, default-only when no deltas, subject-scoped listing). Crate green under `--all-features` clippy `-D warnings`. Part of #5261. Depends on #5262, #5263. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
zetyquickly
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_composition/src/capability_surface_policy.rs`:
- Around line 177-186: Add a caller-level async test that exercises
ScopedLifecyclePolicyCapabilitySurfaceResolver through
CapabilitySurfaceProfileResolver::resolve with a real LoopRunContext, rather
than testing resolve_allow_set() and principal_user_id() separately. In the new
#[tokio::test], build a LoopRunContext with representative scope and actor
inputs, call resolve(), and assert the resulting CapabilityAllowSet reflects the
actor/scope-derived allowlisting so the seam is covered through the dispatch
path.
- Around line 140-155: The scoped-lifecycle store-read fallback in
CapabilitySurfacePolicy::list_effective_installations is intentionally
converting a lookup failure into an empty allowlist, so add an inline `//
silent-ok: ...` annotation at the `Err(error)` fallback with a brief reason that
names the store read/installation lookup operation. Keep the existing debug
logging and fail-closed return in place, but make the intentional silent-ok
decision explicit for this fallback path.
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Line 2830: The capability-policy gate in capability_policy_activated should
not silently treat explicit but unrecognized env values like TRUE or tru as
false; instead, distinguish missing/empty from invalid input and fail startup on
invalid values. Update the logic used by capability_policy_activated and the
code paths around AllowAllCapabilitySurfaceResolver to return the default-off
behavior only when the env var is absent, while surfacing an error and aborting
initialization for any malformed or unsupported value.
🪄 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: a15ad9ea-c370-4728-b9a6-6ec805ab7035
📒 Files selected for processing (4)
crates/ironclaw_reborn_composition/Cargo.tomlcrates/ironclaw_reborn_composition/src/capability_surface_policy.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/runtime.rs
| let effective = match self | ||
| .installations | ||
| .list_effective_installations(subject) | ||
| .await | ||
| { | ||
| Ok(effective) => effective, | ||
| Err(error) => { | ||
| // Denying all (rather than `Err`) keeps a transient store fault | ||
| // — or a tenant that simply has no installations yet — from | ||
| // killing the turn. Logged at debug so it never corrupts the | ||
| // REPL/TUI surface (see CLAUDE.md logging guidance). | ||
| tracing::debug!( | ||
| %error, | ||
| "scoped-lifecycle installation lookup failed; denying all capabilities for this turn" | ||
| ); | ||
| return Ok(CapabilityAllowSet::Allowlist(BTreeSet::new())); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Mark this intentional store-read fallback with silent-ok.
This collapses a scoped-lifecycle store read failure into an empty allowlist. If that fail-closed fallback is intended, the repo fail-loud rule requires an inline // silent-ok: ... reason naming the operation.
Proposed annotation
Err(error) => {
+ // silent-ok: scoped-lifecycle installation lookup fails closed to no capabilities for this turn.
// Denying all (rather than `Err`) keeps a transient store fault📝 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 effective = match self | |
| .installations | |
| .list_effective_installations(subject) | |
| .await | |
| { | |
| Ok(effective) => effective, | |
| Err(error) => { | |
| // Denying all (rather than `Err`) keeps a transient store fault | |
| // — or a tenant that simply has no installations yet — from | |
| // killing the turn. Logged at debug so it never corrupts the | |
| // REPL/TUI surface (see CLAUDE.md logging guidance). | |
| tracing::debug!( | |
| %error, | |
| "scoped-lifecycle installation lookup failed; denying all capabilities for this turn" | |
| ); | |
| return Ok(CapabilityAllowSet::Allowlist(BTreeSet::new())); | |
| let effective = match self | |
| .installations | |
| .list_effective_installations(subject) | |
| .await | |
| { | |
| Ok(effective) => effective, | |
| Err(error) => { | |
| // silent-ok: scoped-lifecycle installation lookup fails closed to no capabilities for this turn. | |
| // Denying all (rather than `Err`) keeps a transient store fault | |
| // — or a tenant that simply has no installations yet — from | |
| // killing the turn. Logged at debug so it never corrupts the | |
| // REPL/TUI surface (see CLAUDE.md logging guidance). | |
| tracing::debug!( | |
| %error, | |
| "scoped-lifecycle installation lookup failed; denying all capabilities for this turn" | |
| ); | |
| return Ok(CapabilityAllowSet::Allowlist(BTreeSet::new())); |
🤖 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/capability_surface_policy.rs` around
lines 140 - 155, The scoped-lifecycle store-read fallback in
CapabilitySurfacePolicy::list_effective_installations is intentionally
converting a lookup failure into an empty allowlist, so add an inline `//
silent-ok: ...` annotation at the `Err(error)` fallback with a brief reason that
names the store read/installation lookup operation. Keep the existing debug
logging and fail-closed return in place, but make the intentional silent-ok
decision explicit for this fallback path.
Source: Coding guidelines
| #[async_trait] | ||
| impl CapabilitySurfaceProfileResolver for ScopedLifecyclePolicyCapabilitySurfaceResolver { | ||
| async fn resolve( | ||
| &self, | ||
| run_context: &LoopRunContext, | ||
| ) -> Result<CapabilityAllowSet, CapabilityResolveError> { | ||
| let user_id = principal_user_id(&run_context.scope, run_context.actor.as_ref()); | ||
| self.resolve_allow_set(&run_context.scope.tenant_id, user_id) | ||
| .await | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add a caller-level test for the resolver seam.
Current tests exercise resolve_allow_set() and principal_user_id() separately, but dispatch uses CapabilitySurfaceProfileResolver::resolve(&LoopRunContext). Add a #[tokio::test] that drives the trait method with a real LoopRunContext and asserts actor/scope-derived allowlisting. As per path instructions, “Test through the caller” when a helper gates dispatch or UI side effects.
🤖 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/capability_surface_policy.rs` around
lines 177 - 186, Add a caller-level async test that exercises
ScopedLifecyclePolicyCapabilitySurfaceResolver through
CapabilitySurfaceProfileResolver::resolve with a real LoopRunContext, rather
than testing resolve_allow_set() and principal_user_id() separately. In the new
#[tokio::test], build a LoopRunContext with representative scope and actor
inputs, call resolve(), and assert the resulting CapabilityAllowSet reflects the
actor/scope-derived allowlisting so the seam is covered through the dispatch
path.
Source: Path instructions
| // `AllowAll` surface so existing flows are unchanged. | ||
| #[cfg(feature = "capability-policy")] | ||
| let capability_surface_resolver: Arc<dyn CapabilitySurfaceProfileResolver> = | ||
| if capability_policy_activated() { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed on invalid capability-policy env values.
An explicit but unrecognized value, e.g. TRUE or tru, currently returns false and falls back to AllowAllCapabilitySurfaceResolver. For a capability exposure gate, invalid config should fail startup; absence can remain the default-off case.
Proposed fix
- if capability_policy_activated() {
+ if capability_policy_activated()? { #[cfg(feature = "capability-policy")]
-fn capability_policy_activated() -> bool {
- std::env::var("IRONCLAW_REBORN_CAPABILITY_POLICY")
- .map(|value| matches!(value.trim(), "1" | "true" | "yes" | "on"))
- .unwrap_or(false)
+fn capability_policy_activated() -> Result<bool, RebornRuntimeError> {
+ let Ok(value) = std::env::var("IRONCLAW_REBORN_CAPABILITY_POLICY") else {
+ return Ok(false);
+ };
+
+ match value.trim().to_ascii_lowercase().as_str() {
+ "1" | "true" | "yes" | "on" => Ok(true),
+ "0" | "false" | "no" | "off" | "" => Ok(false),
+ other => Err(RebornRuntimeError::InvalidArgument {
+ reason: format!(
+ "IRONCLAW_REBORN_CAPABILITY_POLICY has invalid value {other:?}; expected 1/true/yes/on or 0/false/no/off"
+ ),
+ }),
+ }
}Also applies to: 3683-3686
🤖 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/runtime.rs` at line 2830, The
capability-policy gate in capability_policy_activated should not silently treat
explicit but unrecognized env values like TRUE or tru as false; instead,
distinguish missing/empty from invalid input and fail startup on invalid values.
Update the logic used by capability_policy_activated and the code paths around
AllowAllCapabilitySurfaceResolver to return the default-off behavior only when
the env var is absent, while surfacing an error and aborting initialization for
any malformed or unsupported value.
Source: Coding guidelines
…ity resolver module (#5261) Extract the #4544-independent capability-policy engine (delta-store root + local-dev delta store, build_capability_policy_resolver, the PolicyResolver config/approval adapters, and the capability_policy_activated() gate) out of capability_surface_policy.rs into a new capability_policy_engine.rs module. These items depend only on the ironclaw_capability_policy crate and the durable delta store, not on the #4544-coupled scoped-lifecycle availability resolver that stays behind. Turning the engine/availability boundary into a file boundary lets the two ship as separate PRs. Pure move + re-point of references (factory.rs, runtime.rs, runtime/local_dev.rs, profile_approval_authorization.rs doc); no behavior change. Module stays gated #[cfg(feature = "capability-policy")]; feature-off build is byte-unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ngine module (#5261) The acting-principal helper is shared by the availability resolver and the engine's config adapter. Homing it in the #4544-independent engine module (as pub(crate)) means the engine branch carries one canonical copy and the availability resolver imports it — no duplicate when the branches are sliced. Pure move; behavior unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
zetyquickly
changed the title
2 participants
Part of #3288 · the availability-dimension foundation for epic #5261.
The scoped-lifecycle ownership model + durable storage that the capability-policy
availability dimension builds on. (The epic's enforcement engine and control
plane are separate PRs — see #5261; this PR is just the install store.)
User story
A company runs a single multi-tenant IronClaw Reborn instance. An admin installs
and configures shared capabilities (built-in tools, skills, MCP/WASM packages)
once at the tenant level so every user can discover and use them; users still add
private capabilities on top, and a user's private installation takes precedence
for that user without mutating the admin-owned shared installation.
Summary
package installations, with who-can-mutate-each-scope enforcement.
wrappers and CAS-safe upserts.
list_effective_installations(subject)resolves the effective package set fora user (admin-shared → every user in the tenant; user-private → only its owner;
disabled excluded).
Validation
cargo fmt --check;cargo test -p ironclaw_product_workflow_storage --all-features;cargo clippy -p ironclaw_product_workflow_storage --all-targets --features "libsql postgres" -- -D warnings;cargo check -p ironclaw_product_workflow_storage --no-default-features.Where it's used in the epic
The capability availability resolver (#5349) reads this store at the dispatch
seam (installed AND policy-available), and the availability admin REST (on the
control-plane PR #5355) writes it. Those are the only consumers; the policy
engine (#5344) is independent of this store.