Install the packages the catalog already publishes - #7442
Conversation
Skills publish a files list for the scripts and assets they ship, but the catalog entry never deserialized it and the install path passed an empty bundle, so only SKILL.md landed. Files now install alongside it, digest-verified through the same download path and bounded by the limits ironclaw_skills already enforces, and they feed the skill artifact digest while a skill with no files keeps the digest it has today. Tools using HTTP Basic could not publish an extension manifest, so they listed and failed at install; the new basic target carries only the username and the host owns the join and the base64, with a colon or control character rejected at the host boundary, at the channel descriptor, and again at injection.
…tant-derived caps, egress contract tests (#7076)
|
🚅 Deployed to the ironclaw-pr-7442 environment in ironclaw-ci-preview
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds validated host-composed Basic Authorization credentials and extends IronHub skill installation to support validated, verified, concurrently downloaded bundled files with rollback coverage. ChangesBasic credential injection
Bundled skill installation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Manifest
participant HostRuntime
participant Upstream
Manifest->>HostRuntime: Declare Basic credential target
HostRuntime->>HostRuntime: Validate and compose Authorization
HostRuntime->>Upstream: Dispatch request with redacted credential handling
Upstream-->>HostRuntime: Return response
sequenceDiagram
participant IronHub
participant ExtensionManager
participant ScopedInstallation
participant Filesystem
IronHub->>ExtensionManager: Provide manifest and bundled files
ExtensionManager->>ExtensionManager: Download and verify files
ExtensionManager->>ScopedInstallation: Submit bundled installation files
ScopedInstallation->>Filesystem: Materialize normalized paths
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 |
github-actions
Bot
added
scope: docs
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. 🟩 Final result · Completed
Automatic trigger · attempt 1 of 3 · completed in 28m IronLoop completed the review and posted it to GitHub. 🔗 Result |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 IronLoop review
Found an install-path validation defect and a contract-documentation inconsistency.
Findings: 🟠 Medium 1 · 🟡 Low 1
🟠 Medium · Reject file/directory bundle path collisions
Inline on crates/extensions/ironclaw_extension_manager/src/ironhub/catalog.rs:374. See the inline comment for details.
🟡 Low · Synchronize the host-runtime credential contract
The owning host-runtime contract still says only header, query, and path targets are supported and that body targets are out of scope. This change documents Basic, JSON-body, and VAPID targets elsewhere, leaving contradictory guidance for manifest and runtime consumers. Update the owning contract alongside this support.
Validation
- ✅ Bundled-skill installation regression — The focused bounded companion-download and installation test passed.
- ✅ Basic credential egress regression — The focused Basic composition and response-redaction test passed.
- ✅ Architecture boundary suite — The repository architecture test suite passed.
Review details
- Run:
353ff46b-31b1-42e1-9160-8ecab2c45d15 - Workflow: Review
- Attempts: 1
| ))); | ||
| } | ||
| }; | ||
| if !bundle_destinations.insert(destination) { |
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🟠 Medium · Reject file/directory bundle path collisions
The set only rejects equal normalized paths. A signed companion such as `SKILL.md/helper` or `.ironclaw-install.json/helper` passes validation, creates a directory where the generated primary file/sidecar must be written, and makes installation fail later. Likewise, `scripts` plus `scripts/run.py` passes validation. Reject ancestor/descendant destination collisions before download and installation.
There was a problem hiding this comment.
Addressed in 10ab7c9e81: catalog validation now rejects normalized ancestor/descendant destinations, including collisions beneath generated SKILL.md and .ironclaw-install.json, before artifact downloads.
Verification: regression test plus the full ironclaw_extension_manager suite (155 tests), runtime HTTP egress contract (83 tests), architecture suite, formatting, and zero-warning clippy.
There was a problem hiding this comment.
Addressed across 10ab7c9e81 and f027bef5ef: the catalog rejects generated-file and ancestor/descendant collisions before download, and the owning skills-domain validator now enforces the same invariant through the public scoped install path before filesystem writes.
Verification: ironclaw_skills (187 tests), ironclaw_extension_manager (155 tests), architecture suite, formatting, and zero-warning clippy.
Railway preview QA — PASS
Given / When / Then matrix
Status derivation
Regression resultThe previously failing companion-skill case is fixed. The live signed catalog now references a provider-safe path-addressed release asset, the exact artifact downloads with matching size and digest, and Directly naming deferred IronHub capabilities in an initial chat turn produced an unavailable response, so that attempt was not credited. The normal user journey then discovered and invoked the exact Skipped / remaining risk
Cleanup
|
|
Railway QA root cause: the IronHub signed catalog URL for the presentation-generation companion file used a Fix opened with regression coverage: nearai/ironhub#276. After that PR is merged and the signed IronHub catalog is republished, I will rerun the exact |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/domains/ironclaw_skills/src/management/install_bundle.rs (1)
399-420: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDuplicate detection covers only exact normalized equality; ancestor/descendant destinations still pass the domain validator.
destinationsis aBTreeSet<String>and the check is!destinations.insert(destination). A bundle containingscriptsandscripts/run.py, orSKILL.md/helper, passesvalidate_install_bundle_files. The catalog layer incrates/extensions/ironclaw_extension_manager/src/ironhub/catalog.rsnow rejects those, but this crate owns the install-bundle contract andinstall_from_url_for_scopeis public API reachable by other callers. Move the ancestor/descendant rejection here so validation fails before any write, and keep the catalog check as a fast pre-download gate.Confirm no equivalent check already lives in the unshown ranges of this file before applying.
#!/bin/bash # Description: Check whether install_bundle.rs already rejects ancestor/descendant destinations, # and enumerate all callers of the bundle install path. set -euo pipefail fd -t f 'install_bundle.rs' crates/domains/ironclaw_skills | while IFS= read -r f; do echo "== $f ==" rg -n 'strip_prefix|starts_with|BTreeSet|destinations|SKILL_FILE_NAME|INSTALL_METADATA_FILE_NAME' "$f" done echo "== callers of validate_install_bundle_files / SkillInstallRequest.files ==" rg -nP --type=rust -C3 '\bvalidate_install_bundle_files\s*\(|files\s*:\s*&' crates/domains/ironclaw_skills echo "== external callers of install_from_url_for_scope ==" rg -nP --type=rust -C3 '\binstall_from_url_for_scope\s*\(' crates🤖 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/domains/ironclaw_skills/src/management/install_bundle.rs` around lines 399 - 420, Update validate_install_bundle_files around the destinations BTreeSet check to reject any destination that is an ancestor or descendant of another normalized path, not only exact duplicates, before installation writes occur. First verify the unshown portions of install_bundle.rs do not already provide equivalent validation; preserve the catalog’s existing pre-download check and ensure install_from_url_for_scope callers receive the domain validation error.
🤖 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/extensions/ironclaw_extension_manager/src/ironhub/catalog.rs`:
- Around line 356-359: In the bundle destination initialization, replace the
hardcoded "SKILL.md" entry with the publicly exported
ironclaw_skills::SKILL_FILE_NAME constant, while retaining
INSTALL_METADATA_FILE_NAME unchanged.
In `@crates/extensions/ironclaw_extension_manager/src/ironhub/tests.rs`:
- Around line 1368-1370: Update the test fixture setup around skill_file_size
and skill_file_sha to bind skill_file_size to expected_bundled_file.len(), and
add an assertion that expected_bundled_file and tampered_bundled_file have equal
lengths so the test remains focused on checksum verification.
---
Outside diff comments:
In `@crates/domains/ironclaw_skills/src/management/install_bundle.rs`:
- Around line 399-420: Update validate_install_bundle_files around the
destinations BTreeSet check to reject any destination that is an ancestor or
descendant of another normalized path, not only exact duplicates, before
installation writes occur. First verify the unshown portions of
install_bundle.rs do not already provide equivalent validation; preserve the
catalog’s existing pre-download check and ensure install_from_url_for_scope
callers receive the domain validation error.
🪄 Autofix
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: 67a633f7-d1ad-41e3-a326-1b474e2d93da
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (20)
crates/app/ironclaw_architecture_tests/tests/reborn_dependency_boundaries.rscrates/contracts/ironclaw_extension_contracts/src/channel.rscrates/contracts/ironclaw_host_api/src/http.rscrates/contracts/ironclaw_host_api/tests/host_api_contract.rscrates/domains/ironclaw_skills/src/lib.rscrates/domains/ironclaw_skills/src/management.rscrates/domains/ironclaw_skills/src/management/install_bundle.rscrates/domains/ironclaw_skills/src/scoped_management.rscrates/extensions/ironclaw_extension_manager/Cargo.tomlcrates/extensions/ironclaw_extension_manager/src/ironhub/catalog.rscrates/extensions/ironclaw_extension_manager/src/ironhub/model.rscrates/extensions/ironclaw_extension_manager/src/ironhub/service.rscrates/extensions/ironclaw_extension_manager/src/ironhub/tests.rscrates/kernel/ironclaw_host_runtime/src/egress/credential.rscrates/kernel/ironclaw_host_runtime/tests/runtime_http_egress_contract.rscrates/lanes/ironclaw_sandbox/src/plan.rscrates/lanes/ironclaw_sandbox/src/plan_tests.rsdocs/reborn/contracts/extensions.mddocs/reborn/contracts/host-runtime.mddocs/reborn/contracts/secrets.md
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/domains/ironclaw_skills/src/management/install_bundle.rs (1)
30-39: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReplace the raw bundle-path contract with a validated domain type.
normalize_install_bundle_relative_pathreturnsString, whileSkillInstallFileretainsrelative_path: &str. The catalog validates one string value, but the scoped installation contract still transports raw paths. Use one validatedInstallBundleRelativePathnewtype and construct it at the manifest boundary. MakeSkillInstallFilecarry that type.
crates/domains/ironclaw_skills/src/management/install_bundle.rs#L30-L39: define the validated path newtype with the canonical template and replaceSkillInstallFile.relative_path.crates/domains/ironclaw_skills/src/lib.rs#L70-L76: re-export the typed path API.crates/domains/ironclaw_skills/src/management.rs#L24-L30: re-export the typed path API from the contract owner.crates/domains/ironclaw_skills/src/scoped_management.rs#L194-L208: accept typed bundle-file paths through the scoped installation boundary.crates/extensions/ironclaw_extension_manager/src/ironhub/catalog.rs#L360-L392: convert manifest paths once at the external boundary and use typed destinations for collision checks.As per coding guidelines, “Do not pass raw
String,&str,uuid::Uuid, booleans, or magic strings between internal modules.”🤖 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/domains/ironclaw_skills/src/management/install_bundle.rs` around lines 30 - 39, Replace the raw bundle-path contract with a validated InstallBundleRelativePath newtype using the canonical normalization template, and make SkillInstallFile.relative_path carry it. In crates/domains/ironclaw_skills/src/management/install_bundle.rs#L30-L39, define the type and construct it through validation; re-export its API in crates/domains/ironclaw_skills/src/lib.rs#L70-L76 and crates/domains/ironclaw_skills/src/management.rs#L24-L30. Update crates/domains/ironclaw_skills/src/scoped_management.rs#L194-L208 to accept typed paths, and in crates/extensions/ironclaw_extension_manager/src/ironhub/catalog.rs#L360-L392 convert manifest paths once at the boundary and use the validated type for collision checks.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/domains/ironclaw_skills/src/management/install_bundle.rs`:
- Around line 30-39: Replace the raw bundle-path contract with a validated
InstallBundleRelativePath newtype using the canonical normalization template,
and make SkillInstallFile.relative_path carry it. In
crates/domains/ironclaw_skills/src/management/install_bundle.rs#L30-L39, define
the type and construct it through validation; re-export its API in
crates/domains/ironclaw_skills/src/lib.rs#L70-L76 and
crates/domains/ironclaw_skills/src/management.rs#L24-L30. Update
crates/domains/ironclaw_skills/src/scoped_management.rs#L194-L208 to accept
typed paths, and in
crates/extensions/ironclaw_extension_manager/src/ironhub/catalog.rs#L360-L392
convert manifest paths once at the boundary and use the validated type for
collision checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e2b38f4b-2aa1-42f3-8d47-432b992fa620
📒 Files selected for processing (6)
crates/domains/ironclaw_skills/src/lib.rscrates/domains/ironclaw_skills/src/management.rscrates/domains/ironclaw_skills/src/management/install_bundle.rscrates/domains/ironclaw_skills/src/scoped_management.rscrates/extensions/ironclaw_extension_manager/src/ironhub/catalog.rscrates/extensions/ironclaw_extension_manager/src/ironhub/tests.rs
* Install the packages the catalog already publishes Skills publish a files list for the scripts and assets they ship, but the catalog entry never deserialized it and the install path passed an empty bundle, so only SKILL.md landed. Files now install alongside it, digest-verified through the same download path and bounded by the limits ironclaw_skills already enforces, and they feed the skill artifact digest while a skill with no files keeps the digest it has today. Tools using HTTP Basic could not publish an extension manifest, so they listed and failed at install; the new basic target carries only the username and the host owns the join and the base64, with a colon or control character rejected at the host boundary, at the channel descriptor, and again at injection. * fix(ironhub): address package install review findings (nearai#7076) * fix(ironhub): address review round — header-collision rejection, constant-derived caps, egress contract tests (nearai#7076) * refactor(skills): drop unused validate_install_bundle_relative_path wrapper (nearai#7076) * fix(runtime): harden derived credential redaction (nearai#7076) * fix(skills): reject bundle path collisions at domain boundary --------- Co-authored-by: neo-sky <brandon.m.henderson93@gmail.com>
This branch was successfully deployed
3 participants
Supersedes #7076. The original implementation is by @neo-sky; this takeover preserves their authored commit and carries forward the review fixes from @serrrfirat.
Summary
main.mainwithout the stale merge commits from Install the packages the catalog already publishes #7076.Change Type
Linked Issue
Related #7076. This replacement PR gives credit to @neo-sky and is intended to supersede the stale PR while using a same-repository branch for Railway preview deployment.
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warnings(passed before the final rebase; the affected packages were rerun after rebase with--all-targets --all-features -D warnings)cargo build(covered by the stricter Clippy builds and test builds)cargo test -p <owning-crate> --features integration(not applicable: the affected crates do not expose a relevant database-backed integration feature)Test Strategy
User behavior:
Risk areas:
Tests added or updated:
What the tests prove:
Commands run:
cargo fmt --all -- --check— pass after rebase.cargo test -p ironclaw_host_api -p ironclaw_extension_contracts -p ironclaw_skills -p ironclaw_extension_manager— pass after rebase.cargo test -p ironclaw_sandbox plan_tests::— 17 passed after rebase.cargo test -p ironclaw_host_runtime --test runtime_http_egress_contract— 83 passed after rebase.cargo test -p ironclaw_architecture_tests— pass after rebase.python3 scripts/ci/docs_publication_boundary.py— pass after rebase.cargo clippy -p ironclaw_host_api -p ironclaw_extension_contracts -p ironclaw_skills -p ironclaw_extension_manager -p ironclaw_host_runtime -p ironclaw_sandbox --all-targets --all-features -- -D warnings— pass after rebase.cargo clippy --all --benches --tests --examples --all-features -- -D warnings— pass before the final rebase onto currentmain.bash scripts/reborn-e2e-rust.sh architecture-runtime— pass before the final rebase.ironclaw_sandboxsuite before rebase: 215 passed; 3 existing Docker-backed tests could not run because/var/run/docker.sockwas absent. The focused 17-test planner suite passes after rebase.Security Impact
Basic credentials remain host-side and are composed only at mediated runtime egress. The change registers derived base64 and complete Authorization values, including the redaction variants needed when a provider percent-encodes a reflected credential. Bundle paths are normalized before writes, duplicate destinations fail closed, and file/count/aggregate/time limits bound remote catalog input.
Reborn Trust-Boundary Checklist
validate_declarationboundary, while only host runtime code resolves and injects credentials.RuntimeCredentialTargetmatch sites were audited in contracts, host runtime, sandbox planner, and tests.serde(default)fields fail closed or have migration tests. The optional catalogfilesfield preserves empty-list compatibility; every populated entry is fully validated before installation.Database Impact
None. No migration or schema change; existing package manifests without companion files and credential targets other than Basic keep their prior shape and behavior.
Blast Radius
IronHub catalog parsing/downloads, skill bundle installation, extension channel credential declarations, sandbox plan validation, and native runtime HTTP credential injection/redaction. Architecture ceilings move only for the new contract vocabulary:
ironclaw_extension_contracts7,851 → 7,858 andironclaw_host_api18,974 → 18,994.Rollback Plan
Revert this PR. Existing packages without companion files and non-Basic credential targets retain their previous behavior, so rollback does not require a data migration.
Review Follow-Through
Review track: C (security/runtime behavior)