Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds RFC 7617 Basic credential targets and runtime injection. It also adds IronHub bundled skill-file metadata, validation, bounded downloads, installation forwarding, digest handling, and rollback coverage. ChangesBasic credential injection
Bundled skill installation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Manifest
participant CredentialValidator
participant HostRuntime
participant HTTPRequest
Manifest->>CredentialValidator: declare Basic username
CredentialValidator-->>HostRuntime: validated target
HostRuntime->>HTTPRequest: inject Authorization header
HTTPRequest-->>HostRuntime: return redacted response
sequenceDiagram
participant IronHubCatalog
participant IronHubService
participant ScopedManagement
participant SkillBundle
IronHubCatalog->>IronHubService: provide validated bundled-file manifest
IronHubService->>IronHubService: download and verify files
IronHubService->>ScopedManagement: submit SkillInstallFile entries
ScopedManagement->>SkillBundle: materialize bundled files
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size: XL
🔎 Review · PR #7076
Submitted review →Reviewed the complete trusted base-to-head comparison. The catalog companion-file installation path and Basic credential support are consistently validated, bounded, digest-verified, redacted, and covered through production manager/runtime seams. No concrete actionable findings identified. Automatic · PR opened · attempt 1 of 3 · completed in 1m 58s Run details
|
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Review complete · PR #7076
✅ No actionable findings
Reviewed the complete trusted base-to-head comparison. The catalog companion-file installation path and Basic credential support are consistently validated, bounded, digest-verified, redacted, and covered through production manager/runtime seams. No concrete actionable findings identified.
Validation and technical details
- Inspected all 17 changed files and surrounding production call sites across extension contracts/manager, host API/runtime, process sandbox, and skills management.
- Verified trusted comparison refs b89fcd3..2cba19e and checked the diff with git diff --check.
- Traced companion-file path validation, declared and downloaded byte bounds, checksum/size verification, bounded-concurrency downloads, force-install rollback, and scoped bundle publication.
- Traced Basic credential validation from manifest/channel parsing through host injection, Authorization composition, derived-value redaction, and sandbox rejection.
- Reviewed added contract, integration, checksum-failure, limit, rollback, serialization, redaction, and invalid-input tests.
- Could not execute Cargo tests because cargo is unavailable in the review environment.
- Base:
main - Head:
feat/catalog-package-installat2cba19e - Run:
4c24b8e7-2c6a-4fea-a2ab-f87c29f159d8
![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/ironclaw_process_sandbox/src/plan.rs (1)
324-341: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAccept
Basicsandbox credential targets.
SandboxCredentialBinding::validaterejectsRuntimeCredentialTarget::Basic, so a sandbox plan cannot use the advertised Basic credential flow.header_namealso makes the requiredauthorizationkey unreachable, so header deduplication cannot run.
crates/ironclaw_process_sandbox/src/plan.rs#L324-L341: Validate a Basic declaration with the canonical validator and resolve its header key asauthorization.crates/ironclaw_process_sandbox/src/tests.rs#L97-L115: Remove Basic from unsupported targets. Add a caller-driven regression test that accepts valid Basic bindings and rejects duplicateAuthorizationtargets.This violates the declared Basic sandbox integration and the
Test through the callerinvariant. As per coding guidelines, sandbox policy changes require caller-driven coverage.🤖 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_process_sandbox/src/plan.rs` around lines 324 - 341, Update crates/ironclaw_process_sandbox/src/plan.rs lines 324-341 in SandboxCredentialBinding::validate to accept RuntimeCredentialTarget::Basic using the canonical validator, and make header_name resolve Basic targets to the authorization header key. Update crates/ironclaw_process_sandbox/src/tests.rs lines 97-115 to remove Basic from unsupported targets and add caller-driven coverage that accepts valid Basic bindings while rejecting duplicate Authorization targets.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_extension_manager/src/ironhub/catalog.rs`:
- Around line 321-344: Update validate_install_bundle_files to track previously
seen file.path values and reject any duplicate before continuing manifest
validation or digest processing. Keep the existing count, artifact, size, and
relative-path checks unchanged, and return a catalog error identifying the skill
and duplicated bundled path.
In `@crates/ironclaw_host_api/tests/host_api_contract.rs`:
- Line 1: Update the arch-exempt rationale in host_api_contract.rs to explicitly
name the absent dedicated credential-contract fixture module as the missing
aggregation/owner, while preserving the existing Basic credential wire-contract
context and active plan `#4088` reference.
---
Outside diff comments:
In `@crates/ironclaw_process_sandbox/src/plan.rs`:
- Around line 324-341: Update crates/ironclaw_process_sandbox/src/plan.rs lines
324-341 in SandboxCredentialBinding::validate to accept
RuntimeCredentialTarget::Basic using the canonical validator, and make
header_name resolve Basic targets to the authorization header key. Update
crates/ironclaw_process_sandbox/src/tests.rs lines 97-115 to remove Basic from
unsupported targets and add caller-driven coverage that accepts valid Basic
bindings while rejecting duplicate Authorization targets.
🪄 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: 9c699c68-bf96-4c56-8409-553b3e8114fc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (16)
crates/ironclaw_extension_contracts/src/channel.rscrates/ironclaw_extension_manager/Cargo.tomlcrates/ironclaw_extension_manager/src/ironhub/catalog.rscrates/ironclaw_extension_manager/src/ironhub/model.rscrates/ironclaw_extension_manager/src/ironhub/service.rscrates/ironclaw_extension_manager/src/ironhub/tests.rscrates/ironclaw_host_api/src/http.rscrates/ironclaw_host_api/tests/host_api_contract.rscrates/ironclaw_host_runtime/src/egress/credential.rscrates/ironclaw_host_runtime/tests/runtime_http_egress_contract.rscrates/ironclaw_process_sandbox/src/plan.rscrates/ironclaw_process_sandbox/src/tests.rscrates/ironclaw_skills/src/lib.rscrates/ironclaw_skills/src/management.rscrates/ironclaw_skills/src/management/install_bundle.rscrates/ironclaw_skills/src/scoped_management.rs
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Install catalog-published companion files with verified stable digests and enable securely mediated HTTP Basic credentials for extension manifests.
Shape: normal (the PR targets main, is not a stack layer, and the exact local comparison matches all 17 captured GitHub changed-file records); modifiers: none.
Coverage: complete via exact local Git diff. Files: 17 total — production 13, tests 2, config 1, generated/vendor 1, CI 0, docs 0. Five complete packets, no oversized files, no failed reviewers, no limitations.
Stats: 4 findings (from 6 raw, 4 after confidence filtering and overlap deduplication) across 4 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 2.
Conventions
- Medium Preserve the bundled-path validation error (
crates/ironclaw_extension_manager/src/ironhub/catalog.rs:329-334, confidence 100) — anchor:.claude/rules/error-handling.md:17(no diff position — body only)
The newmap_err(|_| ...)discardsSkillManagementError, so the server-side cause is unavailable when catalog validation rejects a bundled path. Bind and retain/log the source before returning the sanitized catalog error.
Also flagged by bugs/Medium: duplicate bundle paths are not rejected, so repeated destinations are downloaded and then silently overwritten during install.
Tests
-
Medium Bounded companion-download concurrency is not tested (
crates/ironclaw_extension_manager/src/ironhub/service.rs:315-329, confidence 100) — anchor:crates/ironclaw_extension_manager/src/ironhub/service.rs:326(no diff position — body only)
The only install test downloads one companion file, so it cannot catch sequential/unbounded regressions or loss of the explicit post-download ordering. Add a gated test with more than eight files and out-of-order completions.
Also flagged by maintainability/Low:bufferedcan preserve input order directly and remove the index/collect/sort protocol. -
Medium Full Basic Authorization value redaction is unverified (
crates/ironclaw_host_runtime/src/egress/credential.rs:417-423, confidence 100) — anchor:crates/ironclaw_host_runtime/src/egress/credential.rs:423
The contract test returns only the encoded token, so it does not protect the newly registered completeBasic <token>redaction value. -
Medium No real extension install exercises a Basic manifest (
crates/ironclaw_host_api/src/http.rs:147-153, confidence 75) — anchor:crates/ironclaw_host_api/src/http.rs:151
Unit/contract coverage exercises the pieces independently, but no real-manager fixture carries a Basic declaration through manifest parsing, installation, lifecycle, and execution.
| @@ -397,6 +408,20 @@ fn apply_credential_injection( | |||
| } | |||
| request.headers.push((name.clone(), injected)); | |||
| } | |||
| RuntimeCredentialTarget::Basic { username } => { | |||
There was a problem hiding this comment.
Medium — Full Basic Authorization value redaction is unverified.
The new runtime code registers both the encoded token and the complete Basic <token> value for redaction, but the added contract test returns only the bare encoded token. Removing the full Authorization value from the derived redaction list would leave the intended header-shaped redaction behavior unprotected.
Fix: Add a contract case whose response contains the complete Basic <base64> value and assert that the whole value is redacted.
There was a problem hiding this comment.
Already addressed in the current head: host_http_egress_composes_and_redacts_rfc7617_basic_credentials serves a response body containing the complete Basic <base64> value and asserts the whole value is redacted (response.body == b"[REDACTED]"); both the encoded token and the full Authorization value are registered for redaction (Ok(vec![encoded, authorization])).
| @@ -144,6 +144,13 @@ pub enum RuntimeCredentialTarget { | |||
| PathPlaceholder { | |||
| placeholder: String, | |||
| }, | |||
| /// Compose an RFC 7617 `Authorization: Basic` header from a manifest-declared | |||
There was a problem hiding this comment.
Medium — No real extension install exercises a Basic manifest.
The tests independently cover target serde, channel validation, and direct host-egress composition, but the real-manager tool fixture has no Basic credential declaration. The user-visible flow this change enables is therefore not covered across manifest parsing, installation, lifecycle, and execution boundaries.
Fix: Add a signed catalog-tool fixture with a v3 Basic credential manifest, install it through the lifecycle manager, and execute a request that asserts the composed Authorization header.
There was a problem hiding this comment.
Covered across the two seams this flow crosses: (1) install — verified_tool_and_skill_install_through_real_managers now installs a signed v3 catalog fixture whose tool manifest declares the Basic credential (injection = { type = "basic", username = "api-user" }) through the real lifecycle managers and asserts the declaration is materialized; (2) execution — host_http_egress_composes_and_redacts_rfc7617_basic_credentials drives the real host egress with the manager-resolved credential material and asserts the composed RFC 7617 header on the wire. The remaining gap (IronHub-catalog install → tool invoke in one test) needs the integration-tier composition harness and is tracked as a follow-up.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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/extensions/ironclaw_extension_manager/src/ironhub/tests.rs`:
- Around line 2740-2749: Update the per-download delay calculation in the
request-handling branch around BoundedDownloadEgress to use saturating
subtraction for ordinal, preventing underflow when the companion-download count
exceeds the delay base. Preserve the existing delay behavior for ordinals below
the base while yielding zero delay for larger ordinals.
- Around line 580-608: Derive the file counts in the total-size validation tests
from MAX_INSTALL_BUNDLE_TOTAL_BYTES and MAX_INSTALL_BUNDLE_FILE_BYTES instead of
hardcoding 32 and 10. Update both the exceeding-total case and the exact-total
case in the affected tests, preserving counts that respectively exceed and
exactly reach the total cap when the constants change.
In `@crates/kernel/ironclaw_host_runtime/src/egress/credential.rs`:
- Around line 411-423: Update the Basic credential injection flow around
RuntimeCredentialTarget::Basic to inspect existing request headers
case-insensitively before adding Authorization, and return the credential error
on any collision. Apply the same single-Authorization enforcement to every
credential injection path, ensuring validation occurs before transport dispatch;
add a caller-level egress test using a mixed-case pre-existing Authorization
header and assert the network is not reached.
🪄 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: 54739bcf-e035-438a-83d8-0d059609bca6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (16)
crates/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.rs
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 (nearai#7076)
6b24337 to
b575075
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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/domains/ironclaw_skills/src/management.rs`:
- Around line 30-31: Confirm the consumer set with the prescribed script, then
remove validate_install_bundle_relative_path from the re-exports in
crates/domains/ironclaw_skills/src/management.rs (lines 30-31) and
crates/domains/ironclaw_skills/src/lib.rs (lines 72-74), while retaining
SkillInstallFile and normalize_install_bundle_relative_path; leave the owning
module’s unit test accessing the validator through super::.
In `@crates/extensions/ironclaw_extension_manager/src/ironhub/tests.rs`:
- Around line 580-589: Add an assertion immediately after computing
total_cap_file_count in the test so it verifies that this derived file count is
within MAX_INSTALL_BUNDLE_FILES, ensuring the subsequent validate_manifest
rejection specifically exercises the total-bytes cap. Keep the existing
rejection assertion and companion test unchanged.
- Around line 1238-1294: Wrap the IronHubCommand::Install execution in
bundled_file_downloads_are_bounded_and_all_files_install with a finite Tokio
timeout, preserving the existing install result assertion while converting an
unfulfilled BoundedDownloadEgress barrier into a test failure instead of a hang.
🪄 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: 7124bb4c-9a2e-4d42-91b6-977bdf3fa695
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (17)
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.rs
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Cargo.toml`:
- Line 38: Update the extension manager’s futures dependency declaration to use
the workspace dependency by replacing the local version specification with
futures.workspace = true. Remove any duplicate local futures declarations, and
apply the same workspace form to futures-util wherever it is declared.
🪄 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: b10e5ff1-4433-4296-bc3e-05f04b19b80b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (17)
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.rs
…-validator re-export, harden bundle tests
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 (3)
crates/domains/ironclaw_skills/src/management.rs (3)
513-532: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winRecheck
MAX_PROMPT_FILE_SIZEafter content repair.The check at Lines 745-748 runs before
ensure_manifest_description. The repair adds adescription:line. Plain-markdown installation also adds synthesized frontmatter at Lines 612-615. A payload just belowMAX_PROMPT_FILE_SIZEcan therefore be persisted above the limit.Apply the size check to the final content in both
prepare_install_contentandupdate_skill, before persistence. Add a caller-level regression test at the limit.As per path instructions: “Validate and bound every new ingress payload before persistence.”
Suggested fix
+fn validate_prompt_file_size(content: &str) -> Result<(), SkillManagementError> { + if content.len() as u64 > MAX_PROMPT_FILE_SIZE { + return Err(SkillManagementError::new( + SkillManagementErrorKind::Resource, + )); + } + Ok(()) +} + +// Call this after every repair or frontmatter synthesis. +validate_prompt_file_size(&final_content)?;Also applies to: 542-550, 612-615, 753-761
🤖 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.rs` around lines 513 - 532, Recheck the size of the final repaired or synthesized content immediately before persistence in both prepare_install_content and update_skill, after ensure_manifest_description and plain-markdown frontmatter insertion have completed. Enforce MAX_PROMPT_FILE_SIZE on that final content and reject oversized payloads through the existing validation path. Add a caller-level regression test covering content at the limit that grows beyond it during repair.Source: Path instructions
206-223: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep the runnable directory contract in sync.
runnable_skill_dirduplicates/workspace/.skillsfromWorkspaceSkillBundleStager::runnable_dir. Add a caller-level test covering activation staging againstrunnable_path_after_activationso future churn catches mismatched literals.🤖 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.rs` around lines 206 - 223, The runnable directory contract lacks a caller-level regression test. Add a test around the activation staging flow that compares the path produced by runnable_skill_dir with WorkspaceSkillBundleStager::runnable_path_after_activation, asserting they resolve to the same directory for a skill name.Source: Path instructions
496-510: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReplace the existing
description:key instead of inserting another one.
ensure_manifest_descriptioninsertsdescription:before the originaldescription:line, leaving duplicate keys. Serde-based YAML parsing does not guarantee this shape is preserved across installs/updates, and the current code does not explicitly handle an already-present empty description. Replace the existing key when present, and preserve the repo invariant: when a helper gates a side effect, test through the real install and update callers.🤖 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.rs` around lines 496 - 510, The ensure_manifest_description flow must replace an existing empty description field rather than inserting a duplicate description key. Update insert_frontmatter_description to locate and replace the manifest’s description entry while preserving valid non-empty descriptions, and ensure the real install and update callers exercise this behavior through their helper gates.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/domains/ironclaw_skills/src/management.rs`:
- Around line 513-532: Recheck the size of the final repaired or synthesized
content immediately before persistence in both prepare_install_content and
update_skill, after ensure_manifest_description and plain-markdown frontmatter
insertion have completed. Enforce MAX_PROMPT_FILE_SIZE on that final content and
reject oversized payloads through the existing validation path. Add a
caller-level regression test covering content at the limit that grows beyond it
during repair.
- Around line 206-223: The runnable directory contract lacks a caller-level
regression test. Add a test around the activation staging flow that compares the
path produced by runnable_skill_dir with
WorkspaceSkillBundleStager::runnable_path_after_activation, asserting they
resolve to the same directory for a skill name.
- Around line 496-510: The ensure_manifest_description flow must replace an
existing empty description field rather than inserting a duplicate description
key. Update insert_frontmatter_description to locate and replace the manifest’s
description entry while preserving valid non-empty descriptions, and ensure the
real install and update callers exercise this behavior through their helper
gates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9bd9fbac-5f46-42c9-bd61-5e72db3bb972
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (4)
crates/domains/ironclaw_skills/src/lib.rscrates/domains/ironclaw_skills/src/management.rscrates/domains/ironclaw_skills/src/management/install_bundle.rscrates/extensions/ironclaw_extension_manager/src/ironhub/tests.rs
|
@ironloopai review |
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. 🟩 Final result · Completed
Manual command by think-in-universe · attempt 1 of 3 · completed in 12m 35s IronLoop completed the review and posted it to GitHub. 🔗 Result |
There was a problem hiding this comment.
🔍 IronLoop review
Found one medium-severity security issue in the new Basic-auth response-redaction path.
Findings: 🟠 Medium 1
🟠 Medium · URL-encoded Basic credentials bypass response redaction
Inline on crates/kernel/ironclaw_host_runtime/src/egress/credential.rs:454. See the inline comment for details.
Validation
- ⚪ Focused test execution — Not run. Static inspection of the credential-injection and response-sanitization paths was sufficient to establish the finding; no focused test command was needed.
Review details
- Run:
0c038264-89f6-45bc-acad-992d969a3c93 - Workflow: Review
- Attempts: 1
| let encoded = base64::engine::general_purpose::STANDARD.encode(joined.as_bytes()); | ||
| let authorization = format!("Basic {encoded}"); | ||
| push_injected_header(request, "Authorization", authorization.clone())?; | ||
| return Ok(vec![encoded, authorization]); |
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🟠 Medium · URL-encoded Basic credentials bypass response redaction
The newly derived Base64 token and full Basic header are registered only as raw strings. Response sanitization performs exact replacements, whereas secret candidates include URL-encoded variants. An endpoint can reflect `Basic dXNlcjpzZWNyZXQ=` as `Basic%20dXNlcjpzZWNyZXQ%3D`; neither raw derived value matches, so the reversible credential reaches runtime-visible output. Generate redaction candidates for both derived values with `redaction_values_for_secret`, and add a percent-encoded reflection regression test.
* 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>
|
Superseded by #7442, which merged on 2026-08-11 and carried this work plus the review fixes onto main. Closing. |
* 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>
3 participants
Rebase and CI
Rebased onto current
main(was three months stale): the two work commits were replayed ontomainand the merge-of-old-main commit dropped. Merge fixes:MixedManifestFixturegainedprompt_url(main's prompt-artifact refactor) and the Basic-manifest fixture now composes with the prompt document; thefuturesdependency moved with the crate tocrates/extensions/ironclaw_extension_manager/.Review round on #7076 addressed: header-collision rejection (
push_injected_header) with two new egress contract tests;saturating_subfor the bounded-download delay; bundle-cap counts derived from theironclaw_skillsconstants; arch-exempt wording names the missing fixture module.Architecture note: contracts size ceilings raised
reborn_contracts_crates_carry_a_checked_size_ceilingrequired raising two §11.2.3 ceilings for this change's contract vocabulary:ironclaw_extension_contracts7_727 → 7_738: the Basic credential target in the channel descriptor vocabulary.ironclaw_host_api18_784 → 18_804: theRuntimeCredentialTarget::Basicdeclaration, its username validation, and wire-format round-trip coverage.Both are declarations only; composition, injection, and enforcement stay in
ironclaw_host_runtime/ironclaw_sandbox.Re-targets #7047 at main. That PR was stacked on
firat/pr-5409-portand auto-closed when #6780 merged and the base branch was deleted; the work never landed. This carries the same change plus the review fixes @serrrfirat pushed to it.Two catalog entries reach install and fail today.
Skills publish a
fileslist 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 limitsironclaw_skillsalready enforces. They feed the skill artifact digest too; 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
basictarget carries only the username — the host owns the join and the base64, so a package cannot ship a pre-encoded credential. The declaration is validated once inRuntimeCredentialTarget::validate_declarationand enforced at the manifest parse, the channel descriptor, and the injection boundary. The sandbox planner rejects the target rather than mis-composing it.The hub half shipped in nearai/ironhub#262 and is live: the catalog now publishes companion files for 21 skills and Basic manifests for wazuh and wordpress, none of which a client can install without this.
Carried over from @serrrfirat's review of #7047: the base64 Basic token and the full
Authorizationvalue are registered for response redaction and the joined plaintext is zeroized; the channel descriptor calls the canonical validator instead of a second copy; companion downloads run through a bounded-concurrency stream.Ported onto main rather than rebased, since #6780 was squash-merged. Three paths needed updating for the WS2 colocate refactor: the
futuresdependency moved toironclaw_extension_manager, and the github fixture path in tests followscrates/extensions/packages/.Tests cover both paths through the real managers, including the composed Basic header. Clippy, fmt and the architecture tests are clean.
Test Strategy
User behavior:
Authorizationvalue.Risk areas:
Tests added or updated:
ironclaw_skillsandironclaw_extension_managersuites pass.What the tests prove:
scripts/run.pyandscripts/./run.pycannot overwrite one destination.Commands run:
cargo fmt --all --check— pass.cargo clippy --all --benches --tests --examples --all-features -- -D warnings— pass.cargo test -p ironclaw_skills— pass (160 unit tests plus routing corpus).cargo test -p ironclaw_extension_manager— pass (145 unit tests plus 2 contract tests).cargo test -p ironclaw_host_runtime --test runtime_http_egress_contract— pass (81 tests).cargo test -p ironclaw_architecture— pass.cargo test --workspace --lib— partial: changed-crate tests pass; workspace run stops on two unrelated network-denied profile tests and three Docker-socket tests because/var/run/docker.sockis unavailable.bash scripts/pre-commit-safety.sh— blocked by pre-existing additions to oversized files outside these review fixes:crates/ironclaw_runner/src/runtime.rsandcrates/ironclaw_threads/tests/session_thread_contract.rs.Security Impact
Basic credentials remain host-side, are composed only at mediated runtime egress, and now register the full authorization value for response redaction. Bundle paths are canonicalized and duplicate destinations fail before writes.
Database Impact
None.
Blast Radius
IronHub catalog validation/downloads, skill bundle installation, extension manifest installation, and runtime HTTP credential redaction.
Rollback Plan
Revert the PR. Existing skill packages without companion files and non-Basic credential targets retain their prior behavior.
Review Follow-Through
The multi-agent review findings were addressed in
563d26df0: duplicate normalized destinations, retained validation diagnostics, deterministic bounded concurrency, complete Basic-value redaction, and real-manager Basic manifest coverage.Review track: C (security/runtime behavior)