Conversation
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
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 refactors the extension manifest parsing and validation by completely removing legacy top-level [[capabilities]] declarations and enforcing that all capabilities are declared under the ironclaw.capability_provider/v1 host API contract section. This change applies to both host-bundled and installed manifests, making contract-based parsing mandatory across all manifest sources. The reviewer provided several constructive suggestions to improve the code, including removing an unused manifest_hash parameter, deserializing directly from TOML value references to avoid cloning, implementing standard error traits for HostApiSectionError, and simplifying string projection helpers in test support files using replacen and replace.
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.
| @@ -54,31 +54,10 @@ impl ExtensionManifestRecord { | |||
| source: ManifestSource, | |||
| host_port_catalog: &HostPortCatalog, | |||
| manifest_hash: Option<ManifestHash>, | |||
There was a problem hiding this comment.
The manifest_hash parameter is completely unused in ExtensionManifestRecord::from_toml. Since this PR already refactors and updates the signature of from_toml across the codebase, you should remove this unused parameter to keep the API clean and avoid dead code.
| let parsed: CapabilityProviderToolsSection = section | ||
| .clone() | ||
| .try_into() | ||
| .map_err(|error: toml::de::Error| error.to_string())?; | ||
| .map_err(|error: toml::de::Error| HostApiSectionError::from(error.to_string()))?; |
There was a problem hiding this comment.
You can deserialize directly from the &toml::Value reference using Deserialize::deserialize instead of cloning the entire section and calling try_into(). This avoids unnecessary allocations and cloning of the TOML value.
let parsed = CapabilityProviderToolsSection::deserialize(section)
.map_err(|error| HostApiSectionError::from(error.to_string()))?;| #[derive(Debug)] | ||
| pub enum HostApiSectionError { | ||
| Manifest(Box<ManifestV2Error>), | ||
| Contract(String), | ||
| } |
There was a problem hiding this comment.
Since HostApiSectionError is a public/shared error type returned by host API contracts, it should implement std::fmt::Display and std::error::Error to be idiomatic and easy to propagate or log. You can derive thiserror::Error directly since thiserror is already a dependency of this crate.
#[derive(Debug, thiserror::Error)]
pub enum HostApiSectionError {
#[error(transparent)]
Manifest(Box<ManifestV2Error>),
#[error("{0}")]
Contract(String),
}| fn project_top_level_capabilities_to_host_api(manifest: String) -> String { | ||
| if !manifest.contains("[[capabilities]]") || manifest.contains("[[host_api]]") { | ||
| return manifest; | ||
| } | ||
| let host_api_block = "[[host_api]]\nid = \"ironclaw.capability_provider/v1\"\nsection = \"capability_provider.tools\"\n\n[capability_provider.tools]\n\n"; | ||
| let idx = manifest.find("[[capabilities]]").expect("checked above"); | ||
| let mut out = String::with_capacity(manifest.len() + host_api_block.len()); | ||
| out.push_str(&manifest[..idx]); | ||
| out.push_str(host_api_block); | ||
| out.push_str(&manifest[idx..]); | ||
| out.replace( | ||
| "[[capabilities]]", | ||
| "[[capability_provider.tools.capabilities]]", | ||
| ) | ||
| } |
There was a problem hiding this comment.
This helper can be simplified significantly by using replacen to replace only the first occurrence of [[capabilities]] with the host_api block and the new capability header, and then using replace to convert any remaining occurrences. This avoids manual index searching, slicing, and capacity allocation, making the code much more readable and maintainable.
| fn project_top_level_capabilities_to_host_api(manifest: String) -> String { | |
| if !manifest.contains("[[capabilities]]") || manifest.contains("[[host_api]]") { | |
| return manifest; | |
| } | |
| let host_api_block = "[[host_api]]\nid = \"ironclaw.capability_provider/v1\"\nsection = \"capability_provider.tools\"\n\n[capability_provider.tools]\n\n"; | |
| let idx = manifest.find("[[capabilities]]").expect("checked above"); | |
| let mut out = String::with_capacity(manifest.len() + host_api_block.len()); | |
| out.push_str(&manifest[..idx]); | |
| out.push_str(host_api_block); | |
| out.push_str(&manifest[idx..]); | |
| out.replace( | |
| "[[capabilities]]", | |
| "[[capability_provider.tools.capabilities]]", | |
| ) | |
| } | |
| fn project_top_level_capabilities_to_host_api(manifest: String) -> String { | |
| if !manifest.contains("[[capabilities]]") || manifest.contains("[[host_api]]") { | |
| return manifest; | |
| } | |
| let first_replace = r#"[[host_api]] | |
| id = "ironclaw.capability_provider/v1" | |
| section = "capability_provider.tools" | |
| [capability_provider.tools] | |
| [[capability_provider.tools.capabilities]]"#; | |
| manifest | |
| .replacen("[[capabilities]]", first_replace, 1) | |
| .replace("[[capabilities]]", "[[capability_provider.tools.capabilities]]") | |
| } |
References
- When canonicalizing a string, perform cheaper validation checks (like length, emptiness, and character set) on a trimmed slice before performing more expensive operations like
replacethat allocate a new string. This avoids unnecessary allocations for already-invalid inputs.
| fn project_top_level_capabilities_to_host_api(manifest: String) -> String { | ||
| if !manifest.contains("[[capabilities]]") || manifest.contains("[[host_api]]") { | ||
| return manifest; | ||
| } | ||
| let host_api_block = "[[host_api]]\nid = \"ironclaw.capability_provider/v1\"\nsection = \"capability_provider.tools\"\n\n[capability_provider.tools]\n\n"; | ||
| let idx = manifest.find("[[capabilities]]").expect("checked above"); | ||
| let mut out = String::with_capacity(manifest.len() + host_api_block.len()); | ||
| out.push_str(&manifest[..idx]); | ||
| out.push_str(host_api_block); | ||
| out.push_str(&manifest[idx..]); | ||
| out.replace( | ||
| "[[capabilities]]", | ||
| "[[capability_provider.tools.capabilities]]", | ||
| ) | ||
| } |
There was a problem hiding this comment.
This helper can be simplified significantly by using replacen to replace only the first occurrence of [[capabilities]] with the host_api block and the new capability header, and then using replace to convert any remaining occurrences. This avoids manual index searching, slicing, and capacity allocation, making the code much more readable and maintainable.
| fn project_top_level_capabilities_to_host_api(manifest: String) -> String { | |
| if !manifest.contains("[[capabilities]]") || manifest.contains("[[host_api]]") { | |
| return manifest; | |
| } | |
| let host_api_block = "[[host_api]]\nid = \"ironclaw.capability_provider/v1\"\nsection = \"capability_provider.tools\"\n\n[capability_provider.tools]\n\n"; | |
| let idx = manifest.find("[[capabilities]]").expect("checked above"); | |
| let mut out = String::with_capacity(manifest.len() + host_api_block.len()); | |
| out.push_str(&manifest[..idx]); | |
| out.push_str(host_api_block); | |
| out.push_str(&manifest[idx..]); | |
| out.replace( | |
| "[[capabilities]]", | |
| "[[capability_provider.tools.capabilities]]", | |
| ) | |
| } | |
| fn project_top_level_capabilities_to_host_api(manifest: String) -> String { | |
| if !manifest.contains("[[capabilities]]") || manifest.contains("[[host_api]]") { | |
| return manifest; | |
| } | |
| let first_replace = r#"[[host_api]] | |
| id = "ironclaw.capability_provider/v1" | |
| section = "capability_provider.tools" | |
| [capability_provider.tools] | |
| [[capability_provider.tools.capabilities]]"#; | |
| manifest | |
| .replacen("[[capabilities]]", first_replace, 1) | |
| .replace("[[capabilities]]", "[[capability_provider.tools.capabilities]]") | |
| } |
References
- When canonicalizing a string, perform cheaper validation checks (like length, emptiness, and character set) on a trimmed slice before performing more expensive operations like
replacethat allocate a new string. This avoids unnecessary allocations for already-invalid inputs.
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | 2fdd7303f360 |
Head: 2fdd7303f360cf518ba4c58ff86f96267fbf4c5b
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found one blocking upgrade compatibility issue in the manifest v2 cutover: persisted host-bundled extension installation state from the previous manifest shape can make Reborn fail to load before the existing bundled-manifest migration can run.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [HIGH] Persisted legacy host-bundled manifests fail store load before migration
Location: crates/ironclaw_reborn_composition/src/extension_host/extension_installation_store.rs:233-239
WireManifestRecord::into_manifest_record now reparses every persisted raw manifest through the new strict ExtensionManifestRecord::from_toml path, which rejects top-level [[capabilities]] for all sources. Existing installation state written by the previous version can contain source = HostBundled records whose raw_toml came from the old bundled assets with top-level capabilities. On upgrade, load_at returns an error while reading state, and build_reborn_services fails before migrate_host_bundled_manifest_hash can replace those records with the new bundled manifests. Add a compatibility path for persisted HostBundled legacy records, or rebuild/migrate those records during state load instead of failing the entire store.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/stale/stalled state while reviewers run.
| let contracts = ironclaw_host_runtime::default_host_api_contract_registry() | ||
| .map_err(invalid_installation_error)?; | ||
| ExtensionManifestRecord::from_toml_with_contracts( | ||
| ExtensionManifestRecord::from_toml( |
There was a problem hiding this comment.
This strict reparse makes old persisted HostBundled records unreadable. Previous versions stored bundled manifests with top-level [[capabilities]]; after this cutover ExtensionManifestRecord::from_toml rejects them before the bundled manifest hash migration can run, so FilesystemExtensionInstallationStore::load_at fails and Reborn startup/config load is blocked for users with existing installed extensions. Please add a load-time compatibility/migration path for those HostBundled legacy records.
|
🚅 Deployed to the ironclaw-pr-5839 environment in ironclaw-ci-preview
|
014e49a to
b081db0
Compare
2fdd730 to
6b2eb09
Compare
60e98ab to
0fda67b
Compare
a635cd9 to
412810e
Compare
…cts everywhere Every manifest now declares its sections through [[host_api]] contracts; the legacy top-level [[capabilities]] form is rejected for every source, host-bundled exactly as installed. All 10 remaining legacy first-party manifests (gmail, google-calendar/docs/drive/sheets/slides, nearai-mcp, notion-mcp, slack, web-access) move onto the ironclaw.capability_provider/v1 section form. One parse entry point remains: ExtensionManifestV2::parse(input, source, catalog, contracts). The contract-free record constructor, the optional- contracts variant, and contract-free ExtensionDiscovery::discover are deleted, along with LegacyTopLevelCapabilitiesForInstalledSource. Host API contracts now raise a typed HostApiSectionError: in-crate contracts (capability provider) preserve precise ManifestV2Error variants (DuplicateEffect, UnknownHostPort, CapabilityIdNotPrefixed, ...) instead of string-flattening them - previously only the deleted legacy path reported typed errors. Domain crates keep redacted reason strings wrapped as HostApiSectionRejected. Production TOML surgery (NEAR AI endpoint audience rewrite) and the shared test fixture converters (legacy_capability_fixture_to_v2 in the dispatcher and host_runtime test support) now emit the host_api form. Contract: docs/reborn/contracts/extensions.md "Cutover (complete)" + converted examples. NEA-25 stack PR 2; no persisted-state impact (installed manifests already could not use the legacy form). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
412810e to
9a6ef20
Compare
…#5850) Atomic roll-up of the 8-PR NEA-25 taxonomy stack onto current main. Extension is the only installable product object; tool/channel/auth are derived capability surfaces; runtime kind controls loading only; manifest projection (v2, host_api contracts) is the sole surface-discovery source of truth. The connectable-channels rail and the parallel `kind` taxonomy are removed and pinned by a zero-legacy gate. slack_bot and slack_personal are retired into one `slack` extension with bounded forward migrations. Extensions wire carries runtime + surfaces, not a conflated kind. Supersedes #5833, #5839, #5842, #5845, #5847, #5848, #5849, #5850. Conflicts with main since the train forked were reconciled preserving main's newer behavior (#5851 unified slack cleanup, #6054 get_conversation_info DM resolution, #5499 extension import, #6057 TS source conventions). provider_identity domain duplication removed; the residual is a legitimate up-layer port adapter. See the PR description for the per-PR crosswalk, resolutions, placement audit, and verification. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Summary
Stack PR 2/7 for NEA-25, on top of #5833. Per Ben's directive the stack carries zero residual legacy code: this PR finishes the manifest v2 cutover instead of keeping the legacy form alive for host-bundled packages.
[[host_api]]contracts. Top-level[[capabilities]]is rejected for everyManifestSource(host-bundled exactly as installed) with an actionable error. The 10 remaining legacy first-party manifests (gmail, google-calendar/docs/drive/sheets/slides, nearai-mcp, notion-mcp, slack, web-access) are converted toironclaw.capability_provider/v1sections.ExtensionManifestV2::parse(input, source, catalog, contracts); deleted: the 3-arg contract-freeparse,parse_with_host_api_contracts,parse_with_optional_host_api_contracts, contract-freeExtensionManifestRecord::from_toml(the_with_contractsvariant becomesfrom_toml), contract-freeExtensionDiscovery::discover, andManifestV2Error::LegacyTopLevelCapabilitiesForInstalledSource.HostApiSectionError { Manifest(ManifestV2Error), Contract(String) }: in-crate contracts preserve precise typed variants (DuplicateEffect,UnknownHostPort,CapabilityIdNotPrefixed, …) through the section channel — previously only the deleted legacy path reported them typed; the host_api path string-flattened everything. Domain crates (product-adapter registry) keep redacted reasons wrapped asHostApiSectionRejected.legacy_capability_fixture_to_v2in dispatcher + host_runtime test support) emit host_api form, so pre-v2 fixtures keep working through one converter instead of ~100 hand edits.docs/reborn/contracts/extensions.md— "Cutover (complete)" replaces the migration-rules section; examples converted; §11 test list extended.No persisted-state impact: installed manifests (
InstalledLocal/RegistryInstalled) already could not use the legacy form; it was exclusively host-bundled assets, synthesis, and test fixtures.Testing
cargo check --workspace --all-targets --all-features— cleanironclaw_extensions(incl. new pins: top-level capabilities rejected for every source; unknown top-level tables →UnreferencedOperationalSection; typed variant passthrough),ironclaw_product_adapter_registry,ironclaw_host_runtime,ironclaw_capabilities,ironclaw_event_projections,ironclaw_mcp,ironclaw_wasm,ironclaw_scripts,ironclaw_dispatcher,ironclaw_reborn_migration,ironclaw_reborn,ironclaw_reborn_composition(1,050 tests incl. NEAR AI renderer path),ironclaw_reborn_cli,ironclaw_architecturecargo clippy --all --benches --tests --examples --all-features— zero warningssandbox_processtests that need a live Docker socket (none on this machine; unrelated to this change), and--features integrationPostgres tier (no DB-shaped change — parse-layer only)🤖 Generated with Claude Code