feat(reborn-ironhub): port IronHub install flow to Reborn - #6754
Conversation
github-actions
Bot
added
scope: docs
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
github-actions
Bot
added
size: XL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe PR adds Reborn IronHub catalog search, signed-manifest verification, artifact validation, skill/tool installation, first-party capability handlers, CLI commands, lifecycle provenance persistence, and trust-aware prompt description handling. Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
🔎 Review · PR #6754
GitHub request failed IronLoop could not complete a required GitHub request. Automatic · PR opened · attempt 1 of 3 · failed after 1m 53s Failure details
|
|
🚅 Deployed to the ironclaw-pr-6754 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: 10
🤖 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_extension_host/src/available_extensions.rs`:
- Around line 517-522: Update from_filesystem_root_with_manifest_sources and the
associated filesystem manifest-loading paths to reject
ManifestSource::HostBundled, accepting only local or registry provenance for
persisted packages. Fail closed before passing the source into manifest parsing,
return the existing appropriate ProductSurfaceFailure, and add a regression test
covering a HostBundled stamp.
In `@crates/ironclaw_extension_host/src/ironhub/capabilities.rs`:
- Around line 205-206: Update both serde_json conversion sites in the capability
handling flow, including the `serde_json::to_value(response)` mapping and the
additional site around lines 217–223, to log the original serde error before
converting it to the existing coarse `FirstPartyCapabilityError`. Preserve the
current error kinds and boundary behavior, but replace discarded-error closures
with logging that includes the serde cause.
In `@crates/ironclaw_extension_host/src/ironhub/model.rs`:
- Around line 9-12: Track a key-rotation plan for MANIFEST_VERIFY_KEYS and add a
production-supported way for service.rs::fetch_manifest to use a second
verification key during rotation, rather than relying only on the single
embedded Ed25519 key. Preserve the existing test-only verify_keys override while
ensuring production can fail over between trusted keys without requiring an
immediate binary release.
In `@crates/ironclaw_extension_host/src/ironhub/service.rs`:
- Around line 595-600: Replace resolve_manifest_url’s ambient std::env::var
lookup with an injected manifest URL supplied through RebornIronHubRuntime or
composition-owned configuration. Apply configuration precedence and fail-closed
validation during bootstrap, and remove the test-only configure_test_catalog
seam so tests use the same injection path.
- Around line 45-50: The process-global MANIFEST_FETCH_LOCKS,
MANIFEST_LAST_SEEN, and INSTALL_LOCKS maps grow without bounds for distinct
user-controlled keys. Update the lock-guard release and related map-management
paths to remove entries whose Arc strong count is 1, or apply the existing
MANIFEST_CACHE_MAX_ENTRIES bound with explicit eviction, while preserving
synchronization and last-seen behavior.
- Around line 406-427: Update the rollback branch in the forced replacement flow
to restore the previous skill through install_from_url_for_scope, passing
self.scope.clone(), Some(name), &previous.content, and the prior source_url
instead of install_for_scope. Preserve the existing restoration error reporting
and original installation error handling while retaining the previous URL-based
install source and its restricted tool permissions.
In `@crates/ironclaw_extension_host/src/ironhub/tests.rs`:
- Around line 86-207: The caller-level coverage around
verified_tool_and_skill_install_through_real_managers is missing failure and
replacement scenarios. Add IronHubService::execute tests using RecordingEgress
for forced tool replacement, including install_registry_package
compensation/rollback, forced skill replacement with restoration, artifact size
and SHA-256 mismatch rejection, and generated_at replay rejection. Assert the
resulting errors, preserved prior installations/content, and relevant
egress/provenance behavior rather than only completed phases.
In `@crates/ironclaw_extension_host/src/product_lifecycle.rs`:
- Around line 1072-1092: The rollback path in the replacement flow currently
calls install under caller, losing the original ExtensionInstallation ownership
and scope. Snapshot the existing installation before self.remove, then restore
that snapshot with restore_installation during compensation, preserving its
tenant/user/agent/project/mission/thread scope and activation state rather than
relying on was_active alone. Add a caller-level test covering a failed registry
replacement for a tenant-shared install.
In `@crates/ironclaw_reborn_cli/src/commands/ironhub.rs`:
- Around line 171-183: Update the IronHub command flow in the runtime block
around execute_reborn_ironhub_command so IronHub actions are represented as a
typed tool request and routed through ToolDispatcher::dispatch(), preserving
existing runtime construction and shutdown behavior. Do not invoke the
extension-host service directly; only add a justified dispatch-exempt annotation
if this command cannot be dispatched.
In `@crates/ironclaw_reborn_composition/src/builtin_capability_policy.toml`:
- Around line 239-256: The catalog grants for builtin.ironhub_search,
builtin.ironhub_info, and builtin.ironhub_install currently use dev_wildcard,
which translates to unrestricted allowed_targets. Verify the host runtime’s
ApplyNetworkPolicy path enforces a tighter catalog-specific egress allowlist for
these grants; if it does not, replace the wildcard network policy with the
appropriate restricted policy and ensure artifact/manifest egress is
constrained.
🪄 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: bfa79d54-bc83-4fe5-bbf8-0e31668d0a39
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (29)
crates/ironclaw_extension_host/Cargo.tomlcrates/ironclaw_extension_host/src/available_extension_import.rscrates/ironclaw_extension_host/src/available_extensions.rscrates/ironclaw_extension_host/src/extension_lifecycle_command.rscrates/ironclaw_extension_host/src/ironhub/capabilities.rscrates/ironclaw_extension_host/src/ironhub/catalog.rscrates/ironclaw_extension_host/src/ironhub/mod.rscrates/ironclaw_extension_host/src/ironhub/model.rscrates/ironclaw_extension_host/src/ironhub/package.rscrates/ironclaw_extension_host/src/ironhub/render.rscrates/ironclaw_extension_host/src/ironhub/service.rscrates/ironclaw_extension_host/src/ironhub/tests.rscrates/ironclaw_extension_host/src/lib.rscrates/ironclaw_extension_host/src/lifecycle_product_service.rscrates/ironclaw_extension_host/src/product_lifecycle.rscrates/ironclaw_extension_host/src/test_support/lifecycle.rscrates/ironclaw_host_api/src/package_lifecycle.rscrates/ironclaw_host_runtime/src/first_party_tools/mod.rscrates/ironclaw_host_runtime/src/first_party_tools/schemas.rscrates/ironclaw_host_runtime/src/first_party_tools/skill_url_install.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_reborn_cli/src/commands/ironhub.rscrates/ironclaw_reborn_cli/src/commands/mod.rscrates/ironclaw_reborn_cli/tests/smoke.rscrates/ironclaw_reborn_composition/src/builtin_capability_policy.tomlcrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_skills/src/scoped_management.rsdocs/reborn-binary.md
| fn resolve_manifest_url() -> String { | ||
| std::env::var("IRONHUB_MANIFEST_URL") | ||
| .ok() | ||
| .filter(|value| !value.trim().is_empty()) | ||
| .unwrap_or_else(|| DEFAULT_IRONHUB_MANIFEST_URL.to_string()) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Catalog endpoint read from ambient env inside a domain crate.
IRONHUB_MANIFEST_URL selects the egress target for the trust root of this feature. Signature verification bounds the damage, but per the Reborn crate guidance configuration layers belong to composition/boot config, not std::env::var at service construction — pass the manifest URL in through RebornIronHubRuntime (or the composition-owned config) so precedence and fail-closed startup apply, and drop the test-only configure_test_catalog seam in favor of the same injection point.
As per coding guidelines: "Keep bootstrap configuration, persisted settings, and encrypted secrets as separate layers; preserve configuration precedence, secret-mediated provider resolution, and fail-closed startup behavior."
🤖 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_extension_host/src/ironhub/service.rs` around lines 595 -
600, Replace resolve_manifest_url’s ambient std::env::var lookup with an
injected manifest URL supplied through RebornIronHubRuntime or composition-owned
configuration. Apply configuration precedence and fail-closed validation during
bootstrap, and remove the test-only configure_test_catalog seam so tests use the
same injection path.
Source: Coding guidelines
There was a problem hiding this comment.
Valid, deferred deliberately. Moving the catalog endpoint from std::env::var to composition-owned config is the right shape per the Reborn config-layering rule, but it changes a config seam rather than fixing a defect, so it needs a maintainer decision on whether it lands here or as a follow-up. Flagged for the reviewer; not changed in e97c124.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
@serrrfirat Acknowledged. The finding remains valid but is intentionally deferred; I’ll leave it unresolved on this PR.
This is a configuration-boundary concern—not a functional defect—and should be tracked separately: inject and validate the catalog endpoint during Reborn composition/bootstrap, preserve config precedence and fail-closed behavior, and remove the configure_test_catalog bypass in favor of that same seam.
Would you like me to open a GitHub follow-up issue?
There was a problem hiding this comment.
Tracked as #6767 — inject the catalog endpoint through composition/boot config, preserve precedence and fail-closed startup, and retire the configure_test_catalog bypass in favor of that seam. Leaving this thread unresolved so the maintainer sees it on the PR.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
@serrrfirat Acknowledged. #6767 captures the required composition/bootstrap configuration boundary work. This thread remains unresolved on this PR as requested; it is not a defect fix in the current diff.
| runtime.block_on(async move { | ||
| let services_input = | ||
| crate::runtime::with_binary_host_extension_bindings(runtime_services.services_input)?; | ||
| let runtime = build_reborn_runtime(RebornRuntimeInput::from_build_input(services_input)) | ||
| .await | ||
| .context("failed to assemble Reborn runtime for IronHub command")?; | ||
| let response = execute_reborn_ironhub_command(&runtime, command) | ||
| .await | ||
| .map_err(anyhow::Error::from)?; | ||
| runtime | ||
| .shutdown() | ||
| .await | ||
| .context("failed to shut down Reborn runtime after IronHub command")?; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Route IronHub CLI actions through ToolDispatcher::dispatch().
Line 177 directly executes the extension-host service, which is constructed with skill and extension managers. This bypasses dispatcher-mediated authority, approval, audit, and policy handling for installs. Route a typed IronHub request through the dispatcher, or add a justified // dispatch-exempt: exception.
As per path instructions, “Everything Goes Through Tools” requires CLI commands to route actions through ToolDispatcher::dispatch().
🤖 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/ironhub.rs` around lines 171 - 183,
Update the IronHub command flow in the runtime block around
execute_reborn_ironhub_command so IronHub actions are represented as a typed
tool request and routed through ToolDispatcher::dispatch(), preserving existing
runtime construction and shutdown behavior. Do not invoke the extension-host
service directly; only add a justified dispatch-exempt annotation if this
command cannot be dispatched.
Source: Path instructions
There was a problem hiding this comment.
Valid, deferred deliberately. Routing IronHub CLI actions through ToolDispatcher::dispatch() is what the capability-dispatch rule requires, and the current code does bypass approval/audit/policy mediation. It's a dispatch-seam refactor rather than a defect fix, so I'd rather a maintainer choose between doing it properly here or landing a justified // dispatch-exempt: annotation. Not changed in e97c124.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
@serrrfirat Acknowledged. The finding remains valid and unresolved: the CLI path violates the “Everything Goes Through Tools” invariant by directly invoking the IronHub service instead of dispatcher mediation.
A maintainer should either route the typed command through ToolDispatcher::dispatch() or document a narrowly justified // dispatch-exempt: exception. Would you like me to open a GitHub follow-up issue for that dispatch-seam refactor?
There was a problem hiding this comment.
Tracked as #6768 — records both acceptable resolutions (typed request through ToolDispatcher::dispatch(), or a narrowly justified // dispatch-exempt: annotation) and flags that it needs a maintainer call. Leaving this thread unresolved deliberately.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
@serrrfirat Acknowledged. #6768 captures the required maintainer decision and both valid resolutions. The finding remains unresolved until the CLI path is dispatcher-mediated or has a narrowly scoped // dispatch-exempt: rationale.
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.49% — 316820 / 370583 lines Per-crate breakdown (60 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)
|
- Preserve URL-sourced skill provenance during forced-replacement rollback. - Restore exact extension installation ownership during compensation. - Reject persisted HostBundled provenance before manifest parsing. - Bound IronHub coordination maps and evict idle keyed locks. - Retain serde error causes in debug logs without changing public error kinds. - Add execute-seam coverage for replacement rollback, integrity checks, and replay rejection. Co-authored-by: neo-sky <brandon.m.henderson93@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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_extension_host/src/available_extensions.rs`:
- Around line 1027-1038: Update the manifest-source handling around the stamp
lookup so a persisted HostBundled source skips only the offending extension
entry and allows the remaining catalog to reload, while still rejecting that
extension’s binding request. Route this case through the existing per-entry
fail-open/skip path described near the InvalidBindingRequest handling, and
preserve or update the regression test to assert that contract.
In `@crates/ironclaw_extension_host/src/ironhub/service.rs`:
- Around line 594-616: Keep the test-only lock/cache introspection helpers
unchanged, but update tests using test_manifest_fetch_lock_exists and
test_install_lock_exists to use fixture-unique URLs and installation keys.
Ensure assertions on absent process-global state cannot conflict with other
tests running in the same test binary.
In `@crates/ironclaw_extension_host/src/ironhub/tests.rs`:
- Around line 727-737: Refactor tool_manifest_json to accept a small fixture
struct, following the existing MixedManifestFixture pattern, and access the
manifest fields through that struct. Remove the
#[allow(clippy::too_many_arguments)] attribute entirely rather than adding an
exemption comment.
In `@crates/ironclaw_skills/src/management.rs`:
- Around line 577-596: The install metadata handling in the skill content flow
must fail closed for unreadable or oversized sidecars. Update
read_install_metadata_bytes and the surrounding source-resolution logic to
distinguish missing metadata from invalid/empty metadata, reject invalid
metadata with an error, and only pass validated metadata to
install_metadata_source; preserve SkillSource::User when the sidecar is
genuinely absent.
🪄 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: e5479c98-815c-4de2-82a4-6684f93cd3e6
📒 Files selected for processing (7)
crates/ironclaw_extension_host/src/available_extensions.rscrates/ironclaw_extension_host/src/ironhub/capabilities.rscrates/ironclaw_extension_host/src/ironhub/service.rscrates/ironclaw_extension_host/src/ironhub/tests.rscrates/ironclaw_extension_host/src/product_lifecycle.rscrates/ironclaw_extension_host/src/test_support/lifecycle.rscrates/ironclaw_skills/src/management.rs
…orting the catalog Fixes an availability regression introduced by e97c124: persisted HostBundled stamps remain rejected, but now skip only the affected extension so valid catalog entries still load. Also removes the IronHub test fixture lint exemption and isolates lock-eviction assertions with fixture-unique identities. Co-authored-by: neo-sky <brandon.m.henderson93@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_extension_host/src/available_extensions.rs (1)
517-522: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKey persisted provenance by
ExtensionId, notString.
manifest_sourcesis an internal map of validated extension identities but accepts raw string keys. Convert persisted keys at the boundary and useBTreeMap<ExtensionId, ManifestSource>here so invalid or unnormalized IDs cannot enter this contract.Suggested adjustment
- manifest_sources: &BTreeMap<String, ManifestSource>, + manifest_sources: &BTreeMap<ExtensionId, ManifestSource>, ... - manifest_sources.get(extension_id.as_str()) + manifest_sources.get(extension_id)This follows the repository rule to use strong types for internal domain 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_extension_host/src/available_extensions.rs` around lines 517 - 522, Update from_filesystem_root_with_manifest_sources to accept manifest_sources as BTreeMap<ExtensionId, ManifestSource> instead of string-keyed entries, and convert persisted/raw keys into validated ExtensionId values at the boundary before calling it. Preserve the existing manifest source lookup behavior while ensuring invalid or unnormalized IDs cannot enter this internal contract.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.
Outside diff comments:
In `@crates/ironclaw_extension_host/src/available_extensions.rs`:
- Around line 517-522: Update from_filesystem_root_with_manifest_sources to
accept manifest_sources as BTreeMap<ExtensionId, ManifestSource> instead of
string-keyed entries, and convert persisted/raw keys into validated ExtensionId
values at the boundary before calling it. Preserve the existing manifest source
lookup behavior while ensuring invalid or unnormalized IDs cannot enter this
internal contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7fe1ae20-ffa4-4220-85e8-222a2082ca0b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (5)
crates/ironclaw_extension_host/src/available_extensions.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_runner/tests/loop_driver_host.rsdocs/reborn/contracts/extensions.mdtests/integration/support/harness/profiles/extension.rs
💤 Files with no reviewable changes (1)
- tests/integration/support/harness/profiles/extension.rs
manifest_sources was BTreeMap<String, ManifestSource>, introduced by aabbc70. The construction site in factory.rs already held a validated ExtensionId and threw the type away (`record.manifest().id.as_str().to_string()`), so an unnormalized key would silently miss every lookup rather than fail — the exact class .claude/rules/types.md exists to prevent. Keyed by ExtensionId end to end: the boundary keeps the typed identity, the catalog lookup drops `.as_str()`, and the two test fixtures construct real ids. Contained to 2 files; no behavior change, and the per-entry host-bundled provenance regression still passes.
|
Applied in 26e3523 — valid, and it was our own regression rather than pre-existing code.
record.manifest().id.as_str().to_string()so an unnormalized or invalid key would silently miss every lookup instead of failing. That is precisely the identity-confusion class Now keyed by 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
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)
4935-5003: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required decomposition tracker for this factory.
This PR adds production wiring to a file exceeding 5,600 lines, but the supplied PR context identifies no decomposition issue or plan. Link a current tracker with an intended split boundary.
As per coding guidelines, “files over 3,000 lines need a decomposition tracking issue.”
🤖 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 4935 - 5003, Update the factory containing the extension installation wiring to reference a current decomposition tracking issue and state the intended split boundary, covering the production wiring added around the extension filesystem, installation store, and filesystem catalog setup. Add only the required tracker reference or nearby documentation; do not alter the runtime behavior.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.
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 4935-5003: Update the factory containing the extension
installation wiring to reference a current decomposition tracking issue and
state the intended split boundary, covering the production wiring added around
the extension filesystem, installation store, and filesystem catalog setup. Add
only the required tracker reference or nearby documentation; do not alter the
runtime behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7f8c9649-ca60-4941-b494-da51ac13fd15
📒 Files selected for processing (2)
crates/ironclaw_extension_host/src/available_extensions.rscrates/ironclaw_reborn_composition/src/factory.rs
|
Declining — the tracker exists and the PR-level threshold is not met. The decomposition tracker is #4469 ("Track Reborn composition factory decomposition", open), with #4922 covering the local-dev capability-composition extraction. So the file-level obligation in The PR-level obligation does not trigger. The same rule reads: "PRs that add > 200 lines need an inline justification." This PR adds +70 / -34 to Worth adding for whoever picks up #4469: a prior investigation concluded the fix is internal modules and just-in-time seams, not more crates — a six-crate extraction was evaluated and abandoned on fan-in evidence. Splitting Also declining on scope grounds: this PR is already +4,868 across 77 files and the size has been raised as a review concern. Restructuring a 5,800-line composition factory inside it would make that materially worse for no correctness gain. 🤖 Addressed by Claude Code |
One conflict: the run_profile host re-export list — import-union (this branch adds CapabilityDescriptionTrust, main adds the LoopRecovery* trio); resolved as the union of both sides. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_loop_host/src/subagent_spawn_port.rs (1)
961-975: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUnscrubbed error cause flows into model-visible
safe_summary.
causeembeds the rawDisplayof the await-edge-writer error and is passed straight intoresolution::failed(..., cause.clone(), ...)as the summary, while only the siblingdetailcopy is run throughcrate::scrub_model_visible_detail. If the underlying error ever carries backend/internal detail, it leaks to the model viasafe_summary, bypassing the scrubbing applied one line later to the same content. As per path instructions, "map internal identifiers, tracebacks, provider responses, filesystem paths, runtime output, credential material, and transport errors to stable sanitized user-facing categories/messages... Preserve redacted details only in logs and durable events according to their redaction contracts."🛡️ Proposed fix: scrub once, keep summary generic
- let cause = format!("subagent spawn scope recovery in progress: {error}"); - return Ok(resolution::failed( - FailureKind::Transient, - cause.clone(), - CapabilityFailureDetail::Diagnostic { - text: crate::scrub_model_visible_detail(cause), - }, - )); + let scrubbed = crate::scrub_model_visible_detail(format!( + "subagent spawn scope recovery in progress: {error}" + )); + return Ok(resolution::failed( + FailureKind::Transient, + "subagent spawn scope recovery in progress".to_string(), + CapabilityFailureDetail::Diagnostic { text: scrubbed }, + ));🤖 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_loop_host/src/subagent_spawn_port.rs` around lines 961 - 975, Update the error handling in the await-edge-writer scope recovery check so the raw error is never used as the failed resolution summary. Scrub the composed cause once for diagnostic detail, and provide a stable generic user-facing summary to resolution::failed while preserving the scrubbed text in CapabilityFailureDetail::Diagnostic.Source: 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.
Outside diff comments:
In `@crates/ironclaw_loop_host/src/subagent_spawn_port.rs`:
- Around line 961-975: Update the error handling in the await-edge-writer scope
recovery check so the raw error is never used as the failed resolution summary.
Scrub the composed cause once for diagnostic detail, and provide a stable
generic user-facing summary to resolution::failed while preserving the scrubbed
text in CapabilityFailureDetail::Diagnostic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 19a32e98-c413-42f8-bdc5-634fe2abc0c7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (12)
crates/ironclaw_agent_loop/src/executor/capability_helpers.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/test_support/mod.rscrates/ironclaw_extension_host/Cargo.tomlcrates/ironclaw_loop_host/src/capability_port.rscrates/ironclaw_loop_host/src/subagent_spawn_port.rscrates/ironclaw_loop_host/src/subagent_spawn_port/tests.rscrates/ironclaw_runner/src/tool_disclosure_port.rscrates/ironclaw_turns/src/run_profile/host/capability.rscrates/ironclaw_turns/src/run_profile/host/mod.rscrates/ironclaw_turns/src/run_profile/mod.rstests/integration/support/doubles/recording_test_capability_port.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_extension_host/src/product_lifecycle.rs (1)
1014-1107: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDefer credential revocation until registry replacement succeeds.
self.remove(...)in the forced replacement path runsrevoke_exclusive_credentials, which invokescleanup_for_lifecycleand can delete/quarantine provider credential material. Ifinstall_and_activate_registry_packagelater fails, compensation restores the catalog, installation row, and activation only; it never re-provisions the revoked credential. This violates the destructive-operation rollback invariant: persisted state must remain reconstructible after interruption, and credential cleanup must not leave a restored extension active with unusable provider credentials.🤖 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_extension_host/src/product_lifecycle.rs` around lines 1014 - 1107, Defer destructive credential cleanup during forced replacement in the surrounding replacement flow: do not call `self.remove(package_ref.clone(), scope, Some(caller))` before `install_and_activate_registry_package` succeeds, or otherwise use a non-revoking removal path. Ensure failed replacement compensation can restore the prior catalog, installation scope, activation, and usable provider credentials, while retaining the existing cleanup behavior after a successful replacement.
🤖 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.
Outside diff comments:
In `@crates/ironclaw_extension_host/src/product_lifecycle.rs`:
- Around line 1014-1107: Defer destructive credential cleanup during forced
replacement in the surrounding replacement flow: do not call
`self.remove(package_ref.clone(), scope, Some(caller))` before
`install_and_activate_registry_package` succeeds, or otherwise use a
non-revoking removal path. Ensure failed replacement compensation can restore
the prior catalog, installation scope, activation, and usable provider
credentials, while retaining the existing cleanup behavior after a successful
replacement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ed51e0b2-0b59-47f1-b526-34b5f6f1ef7d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (18)
crates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/executor/tests/support.rscrates/ironclaw_extension_host/Cargo.tomlcrates/ironclaw_extension_host/src/available_extension_import.rscrates/ironclaw_extension_host/src/available_extensions.rscrates/ironclaw_extension_host/src/extension_lifecycle_command.rscrates/ironclaw_extension_host/src/lib.rscrates/ironclaw_extension_host/src/lifecycle_product_service.rscrates/ironclaw_extension_host/src/product_lifecycle.rscrates/ironclaw_extension_host/src/test_support/lifecycle.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/tests/tool_surface_contract.rscrates/ironclaw_loop_host/src/capability_port.rscrates/ironclaw_loop_host/src/capability_surface_filter.rscrates/ironclaw_loop_host/src/external_tool_capability.rscrates/ironclaw_loop_host/src/subagent_spawn_port.rscrates/ironclaw_loop_host/src/subagent_spawn_port/tests.rscrates/ironclaw_loop_host/src/synthetic_capability.rs
💤 Files with no reviewable changes (1)
- crates/ironclaw_extension_host/Cargo.toml
* feat(reborn): port IronHub install flow * fix(reborn-ironhub): preserve skill install source and scope on rollback - Preserve URL-sourced skill provenance during forced-replacement rollback. - Restore exact extension installation ownership during compensation. - Reject persisted HostBundled provenance before manifest parsing. - Bound IronHub coordination maps and evict idle keyed locks. - Retain serde error causes in debug logs without changing public error kinds. - Add execute-seam coverage for replacement rollback, integrity checks, and replay rejection. Co-authored-by: neo-sky <brandon.m.henderson93@gmail.com> * fix(reborn-ironhub): skip host-bundled stamps per entry instead of aborting the catalog Fixes an availability regression introduced by e97c124: persisted HostBundled stamps remain rejected, but now skip only the affected extension so valid catalog entries still load. Also removes the IronHub test fixture lint exemption and isolates lock-eviction assertions with fixture-unique identities. Co-authored-by: neo-sky <brandon.m.henderson93@gmail.com> * fix(turns): trust verified catalog descriptions instead of denying the whole prompt Installing Attio from the signed IronHub catalog exposed an official description containing API key and Bearer authentication vocabulary. Prompt validation treated that trusted text as an unsafe summary and denied every subsequent turn. Carry verified catalog provenance into capability descriptors and route it through a trusted prompt-text surface, following the certified-skill fix from nearai#5169/nearai#5258. Structural checks still apply, while invalid untrusted descriptors are omitted individually with host diagnostics naming the capability and matched pattern. Co-authored-by: neo-sky <brandon.m.henderson93@gmail.com> * test(golden): re-bless capability surface hashes after the description-trust field Adding CapabilityDescriptionTrust to CapabilityDescriptorView changes the capability surface fingerprint, so the golden payload snapshots carry a new surface sha256. Verified the change is hash-only: all 7 changed content lines are byte-identical once the surface hash is normalized, with zero other content deltas. The trust field does not appear in model-visible prompt content — the capability list and every description are unchanged. Only insta's stale assertion_line metadata was additionally dropped. * fix(ironhub): return complete, self-describing search results instead of a silent truncation (nearai#6808) IronHub search returned a result-reference prefix, leading the agent to report attio missing even though it was present in the signed catalog. Return compact catalog projections with explicit completeness metadata and a bounded, unmistakably incomplete fallback. Closes nearai#6788 Co-authored-by: neo-sky <brandon.m.henderson93@gmail.com> * test(integration): cover the install-then-turn prompt-denial incident at the turn seam Drive registry-verified and local extension descriptions through the real product workflow, scheduler, agent loop, and model request boundary. This closes the Attio incident gap by proving verified Bearer-header wording survives intact while one unsafe local prompt entry degrades without collapsing the remaining capability surface. Co-authored-by: neo-sky <brandon.m.henderson93@gmail.com> * fix(ironhub): measure catalog_total inside the truncation budget Follow-up to b218c05. The bounded-fallback path sized `Self::incomplete(...)` against MAX_SEARCH_RESPONSE_BYTES while that shape still had `catalog_total: None` — omitted from JSON by skip_serializing_if — and then assigned Some(..) to the value actually returned. The emitted payload was therefore ~20 bytes larger than the budget that admitted it, so a truncated result could exceed the cap it exists to enforce. `incomplete` now takes `catalog_total`, so the shape measured is exactly the shape emitted. Extended the existing oversized-catalog test rather than adding a fourth search test (it already owns the truncated path): it now pins catalog_total on both the struct and the serialized payload, alongside its existing size bound. * refactor(extension-host): key persisted manifest sources by ExtensionId manifest_sources was BTreeMap<String, ManifestSource>, introduced by aabbc70. The construction site in factory.rs already held a validated ExtensionId and threw the type away (`record.manifest().id.as_str().to_string()`), so an unnormalized key would silently miss every lookup rather than fail — the exact class .claude/rules/types.md exists to prevent. Keyed by ExtensionId end to end: the boundary keeps the typed identity, the catalog lookup drops `.as_str()`, and the two test fixtures construct real ids. Contained to 2 files; no behavior change, and the per-entry host-bundled provenance regression still passes. * fix(turns): keep prompt validation errors compact --------- Co-authored-by: neo-sky <brandon.m.henderson93@gmail.com> Co-authored-by: serrrfirat <firatsertgoz@alumni.sabanciuniv.edu> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Summary
Ports the IronHub catalog and install flow to the Reborn stack. This supersedes #4479, which was written against a codebase that has since moved 903 commits — the extension lifecycle it hooked into was relocated out of
ironclaw_reborn_compositioninto the newironclaw_extension_hostcrate (#6116, #6616), andironclaw_product_workflowwas deleted outright (#6583). Rather than force-apply a stale diff across delete-vs-modify conflicts, the integration is reimplemented natively against the current extension-host model. No IronHub code existed on main, so nothing is superseded or duplicated.What the change adds:
ironhub/catalog.rs) — Ed25519 manifest verification viaverify_strict, with hex/length validation of pinned verify keys.ironhub/service.rs) — HTTPS fetch through the host egress seam, gated on three independent checks before bytes are used: response body size cap, exact declared-size match, and SHA-256 digest comparison. Declared digests are validated as 64 hex characters before any fetch.reborn.extension_manifest.v3WASM extension packages, with force-replacement rollback.ironclaw ironhub search/list/info/installCLI plus first-party model-callable search/info/install capabilities.The
ironhub/module lives incrates/ironclaw_extension_host/alongside the lifecycle it integrates with;ironclaw_reborn_compositiontakes only wiring touches (factory.rs,runtime.rs,builtin_capability_policy.toml).Validation
Run locally on this branch (parent is the main tip, so there is no merge surface):
cargo fmt --all -- --check— passedcargo clippy --all --benches --tests --examples --all-features -- -D warnings— passedironclaw_extension_host294 tests,ironclaw_host_api248 tests + contracts,ironclaw_skills229 tests + routing corpus — passedcargo test— 460 unit + 6 extension tests passedTwo gates could not be verified locally and rely on CI:
Operation not permitted).Neither is a known code failure, but neither is confirmed green here.
Review guidance
The security-critical surface is
ironhub/catalog.rs(signature verification) anddownload_verifiedinironhub/service.rs(size + digest enforcement) — worth the closest look, since this path installs remote code. The bulk of the remaining diff is catalog modeling, rendering, and tests.Closes #4479.
Credit
The original implementation is @neo-sky's work in #4479 — this PR re-implements the same design against the current extension-host layout. Authorship is carried in the commit trailer.