feat: define trusted skills distribution and installation model for corvus#311
Merged
yacosta738 merged 7 commits intoMar 25, 2026
Conversation
…model for Corvus Implement a three-tier trust model (Official/Local/ThirdParty) for Corvus skills with an official catalog system, closing the security gap where untrusted third-party skills were auto-loaded into every agent prompt. Phase 1 — Trust Foundation: - SkillTrust enum derived from SkillOrigin (prevents privilege escalation) - Open-skills default OFF with deprecation warning - Skills lockfile (skills.lock) with SHA-256 integrity hashes - Trust-aware prompt rendering with third-party caution notes - allowed-tools gating for third-party skills - --trust CLI flag for install consent Phase 2 — Official Skills Catalog: - Catalog index embedded at build time (offline-first) - corvus skills install <name> resolves from catalog with Official trust - corvus skills search/list --catalog/update/discover commands - corvus skills lock repair for lockfile maintenance - SKILL.toml deprecation warning, SkillForge trust boundaries - auto_integrate deprecated (default off) New files: trust.rs, frontmatter.rs, lockfile.rs, catalog.rs, catalog_index.toml, build.rs Modified: mod.rs, prompt.rs, schema.rs, lib.rs, main.rs, integrate.rs, skillforge/mod.rs, config/mod.rs, Cargo.toml, channels/mod.rs, onboard/wizard.rs Spec: openspec/specs/skills-trust/spec.md (R1-R13, 38 scenarios) Refs: #261
…anner, and sandboxing Phase 3 of trusted skills distribution — security hardening: P0 Security Critical: - Remove open-skills code (~200 lines, deprecated since Phase 1) - Remove SKILL.toml loading (~100 lines, deprecated since Phase 2) - Content integrity verification on load (SHA-256 hash comparison) - ThirdParty hash mismatch → tools disabled (instruction-only mode) P1 High Value: - Skill name validation per Agent Skills standard ([a-z0-9-], 1-64 chars) - Prompt injection scanner (5 categories, scoring-based, configurable threshold) - Scanner blocks suspicious third-party installs unless --trust flag - Deferred Phase 2 tests (catalog resolution, lockfile repair, TOML rejection) P2 Medium Value: - Tool sandboxing for third-party skills (path traversal prevention) - Working directory restriction to skill directory - Symlink escape detection New files: validation.rs, scanner.rs, sandbox.rs Modified: mod.rs, lockfile.rs, catalog.rs, schema.rs Deleted: ~300 lines of deprecated open-skills and SKILL.toml code Net code reduction in mod.rs (530 deletions vs 496 additions). Spec: openspec/specs/skills-trust/spec.md (R1-R20, 35+ scenarios) Refs: #261
…ty policy, lifecycle, and official catalog Formalize the 4 child workstreams from issue #261 as product contract specifications, closing the planning gap between implementation and product-level definitions. Contracts added: - local-skills-ux: directory layout, SKILL.md format, validation rules - third-party-policy: consent model, scanner/trust gates, sandboxing - skill-lifecycle: lockfile registry, install/update/remove flows, agent-driven install policy (Official only, opt-in) - official-catalog: repository model, index format, 5-point review standard, embedded+cache offline strategy These contracts document behavior already shipped in Phases 1-3. No code changes — documentation only. Refs: #261
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements an official skills catalog (build-time embed + cache + network fallback), a lockfile-based skills metadata system, trust/origin modeling (Official/Local/ThirdParty), SKILL.md frontmatter parsing, integrity verification, prompt-injection scanning, sandboxing for third‑party tools, CLI subcommands (search/update/discover/lock repair), and related refactors across the agent-runtime and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Catalog
participant Lockfile
participant Scanner
participant Loader
participant Prompt
rect rgba(100,200,255,0.5)
Note over User,CLI: Bare-name install → Official
User->>CLI: corvus skills install core-chat
CLI->>Catalog: resolve_index(workspace, config)
Catalog-->>CLI: CatalogIndex entry
CLI->>CLI: validate_skill_name("core-chat")
CLI->>Lockfile: write_lock_entry(name, LockEntry{trust="Official", source="catalog:core-chat", ...})
end
rect rgba(255,150,100,0.5)
Note over User,CLI: URL install → ThirdParty (tools)
User->>CLI: corvus skills install https://... --trust
CLI->>Scanner: scan_skill_content(SKILL.md)
alt score > threshold
CLI-->>User: blocked (high injection risk)
else
CLI->>CLI: prompt/trust gating (--trust)
CLI->>Lockfile: write_lock_entry(... trust="ThirdParty", source="https://...")
end
end
rect rgba(150,200,100,0.5)
Note over User,Loader: Load with integrity check & prompt injection handling
User->>Loader: load_skills_with_config(workspace, verify_integrity=true)
Loader->>Lockfile: read_lockfile()
loop each skill
Loader->>Loader: compute_content_hash(SKILL.md)
Loader->>Lockfile: verify_integrity(...)
alt ThirdParty + mismatch or scan exceeds
Loader->>Loader: warn + clear allowed_tools (instruction-only)
else
Loader->>Loader: set tool.sandboxed per trust
end
end
Loader-->>Prompt: skills_to_prompt(sorted_by_trust)
end
rect rgba(200,100,255,0.5)
Note over User,CLI: Lockfile repair
User->>CLI: corvus skills lock repair
CLI->>Lockfile: repair_lockfile(workspace) -> RepairSummary
Lockfile-->>CLI: {added, removed, updated, unchanged}
CLI-->>User: report summary
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
This was
linked to
issues
Mar 24, 2026
Contributor
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-03-25 to 2026-03-25 |
Deploying corvus with
|
| Latest commit: |
967bc37
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4c54eebc.corvus-42x.pages.dev |
| Branch Preview URL: | https://feature-261-define-trusted-s.corvus-42x.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 36
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clients/agent-runtime/src/skillforge/integrate.rs (1)
50-97:⚠️ Potential issue | 🟠 MajorEscape frontmatter scalar values before writing YAML.
At Line 55–60 and Line 89–94, external metadata is inserted as raw YAML scalars. Values containing
:, newline, or YAML markers can corruptSKILL.mdfrontmatter or alter structure.🔧 Proposed fix
impl Integrator { @@ fn generate_md(&self, c: &ScoutResult) -> String { let lang = c.language.as_deref().unwrap_or("unknown"); - let tags_yaml = format!(" - discovered\n - {lang}"); + let tags_yaml = format!(" - discovered\n - {}", yaml_quote(lang)); + let name = yaml_quote(&c.name); + let description = yaml_quote(&c.description); + let owner = yaml_quote(&c.owner); format!( r#"--- name: {name} description: {description} version: 0.1.0 author: {owner} tags: {tags} --- @@ "#, - name = c.name, - description = c.description, - owner = c.owner, + name = name, + description = description, + owner = owner, tags = tags_yaml, title = c.name, url = c.url, lang = lang, stars = c.stars, license = if c.has_license { "yes" } else { "unknown" }, ) } } + +fn yaml_quote(value: &str) -> String { + format!("'{}'", value.replace('\'', "''")) +}As per coding guidelines
**/*: Security first, performance second. Validate input boundaries, auth/authz implications, and secret management.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/skillforge/integrate.rs` around lines 50 - 97, The generate_md function is inserting unescaped external metadata (fields on ScoutResult like name, description, owner, url, language/lang, stars, license and the tags_yaml variable) directly into YAML frontmatter which can be broken by colons, newlines or YAML markers; fix by escaping or quoting all frontmatter scalar values before formatting (e.g., serialize each scalar with a YAML-safe escaper or use a YAML serializer for the frontmatter block rather than string interpolation), ensure tags_yaml is constructed as a safe YAML sequence (escape tag strings or build as a sequence), and replace direct uses of c.name, c.description, c.owner, c.url, lang, c.stars and the license expression in generate_md with the escaped/serialized versions so SKILL.md frontmatter cannot be corrupted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/agent/prompt.rs`:
- Around line 230-255: The prompt builder writes raw skill metadata into
XML-like blocks (see loop over sorted_skills and fields skill.name,
skill.description, and location.display()), which allows injected tags/control
chars to break the prompt; fix by escaping/encoding those fields before writing
them (introduce and call a helper like escape_xml or escape_xml_str on
skill.name, skill.description and location.display() outputs in the writeln
branches), ensuring all values inserted into the <skill> elements are properly
XML-escaped/encoded while preserving the rest of the formatting and existing
trust-based branch logic.
In `@clients/agent-runtime/src/config/schema.rs`:
- Around line 238-266: Add runtime validation for the SkillsConfig fields inside
validate_for_runtime(): ensure catalog_repo_url (from
SkillsConfig::catalog_repo_url) is parsed and enforces a tight scheme/host
policy (reject non-https schemes and disallow unsafe hosts like localhost,
127.0.0.1, file:// and other non-remote hosts; if you have an allowlist use it)
and return an error on invalid or disallowed URLs, and validate
catalog_cache_ttl_hours (SkillsConfig::catalog_cache_ttl_hours) to reject
Some(0) (or any 0 value) with a clear validation error; place these checks
alongside the other runtime validations in validate_for_runtime() so defaults
remain secure-by-default.
In `@clients/agent-runtime/src/lib.rs`:
- Around line 144-147: Update the doc comment for the Install command's source
field (Install.source) to reflect the new contract: change the help text from
"Source URL or local path" to include catalog names (e.g., "Source URL, local
path, or catalog name") so the CLI arg documentation matches accepted inputs.
In `@clients/agent-runtime/src/skillforge/integrate.rs`:
- Around line 148-175: The test uses a fixed temp directory (tmp) which can
cause races and leftover state; update the test to create a unique temporary
directory (e.g., via tempfile::TempDir or append a UUID/timestamp/PID) and pass
that path into Integrator::new instead of the hardcoded "corvus-test-integrate",
update references to tmp, and ensure cleanup uses TempDir drop or remove_dir_all
on the unique path; refer to Integrator::new, sample_candidate(), and the tmp
variable in the test to locate the change.
In `@clients/agent-runtime/src/skillforge/mod.rs`:
- Around line 320-340: The test currently uses SkillForge::new which forces
auto_integrate=false so bad_path is never used; rename this test to indicate
it's a routing test (e.g., integrates_to_manual_review_routing) and then add a
separate module-local test that constructs SkillForge directly via
SkillForgeConfig (set enabled:true, auto_integrate:true, output_dir: bad_path)
to exercise the failure path when calling
forge.integrate_results(&[eval_result]); in that new test assert that failed > 0
(and auto_integrated/manual_review behave as expected) so the integration
failure case is covered.
In `@clients/agent-runtime/src/skills/catalog_index.toml`:
- Around line 8-10: The catalog_index.toml currently contains placeholder
provenance fields (generated_at, repo_url, commit); update the build/publish
flow to populate generated_at with the actual ISO8601 snapshot timestamp,
repo_url with the real repository URL, and commit with the real git SHA for the
snapshot (and ensure the code that consumes this file—look for any
loader/validator that reads catalog_index.toml—is modified to validate these
fields and reject or fail CI when generated_at is unchanged or commit is all
zeros); add a validation check that fails the build or refuses to ship if commit
== "0000000000000000000000000000000000000000" or generated_at is the static
placeholder.
In `@clients/agent-runtime/src/skills/catalog.rs`:
- Around line 97-101: The code that sets index_url ignores the configured value
because config.catalog_repo_url.as_deref().map(|_| OFFICIAL_INDEX_URL) always
maps any Some to OFFICIAL_INDEX_URL; change the logic so the actual config value
is used (e.g., use
config.catalog_repo_url.as_deref().unwrap_or(OFFICIAL_INDEX_URL)) so index_url
receives the custom URL when provided; update the assignment that defines
index_url (referencing config.catalog_repo_url and OFFICIAL_INDEX_URL)
accordingly, or if the intent is to derive an index URL from the repo URL,
replace the map with a mapping that transforms the repo URL string into the
desired index URL rather than returning OFFICIAL_INDEX_URL unconditionally.
- Around line 137-172: The blocking reqwest::blocking::Client call inside
try_fetch_index (invoked from skills::handle_command and async
handle_cli_command) blocks the tokio runtime; change try_fetch_index to use the
async reqwest::Client (make the function async and await .get(url).send().await
and resp.text().await) or, if you must keep the blocking client, move the entire
blocking section into tokio::task::spawn_blocking and await its result; update
callers (skills::handle_command / handle_cli_command) to await the new async
try_fetch_index signature or handle the JoinHandle result, and keep existing
timeout constant INDEX_FETCH_TIMEOUT_SECS when configuring the async client or
preserving the blocking behavior inside spawn_blocking.
In `@clients/agent-runtime/src/skills/lockfile.rs`:
- Around line 158-164: The current_hash calculation redundantly checks
skill_md.exists() even though the loop already continues when
!skill_md.exists(); replace the if/else block in the current_hash assignment
with a direct read-and-map: attempt std::fs::read(&skill_md).ok() and then map
the result into compute_content_hash(&c) to produce Option<Hash>, keeping the
variable name current_hash and use compute_content_hash unchanged; this removes
the unnecessary exists() branch while preserving the same error-tolerant
behavior.
In `@clients/agent-runtime/src/skills/mod.rs`:
- Around line 124-139: The current injection check only scans
skill.prompts.first(), which misses later prompt entries; update the ThirdParty
scanner logic (in the block referencing skill.prompts,
scanner::scan_skill_content, scanner::DEFAULT_SCAN_THRESHOLD, and
skill.allowed_tools.clear) to scan the full prompt content instead of just the
first entry — either by iterating over all entries and failing if any
scan.exceeds_threshold(...) or by concatenating all prompts (or using the raw
SKILL.md content as produced by load_skill_md) and running a single scan; keep
the same warning message and behavior (clearing allowed_tools) when the
threshold is exceeded.
- Around line 1290-1309: The code currently calls
std::fs::remove_dir_all(&skill_dir) before attempting the git clone
(std::process::Command::new("git") ... .status()), which causes unrecoverable
data loss if clone_status fails; change the flow to clone into a temporary
directory (e.g., skill_dir.with_extension or skill_dir.join(".tmp")), check
clone_status for success, then atomically replace the existing skill_dir by
renaming the temp directory into place (std::fs::rename or platform-appropriate
atomic replace), and only remove the old directory after a successful swap;
ensure cleanup of the temp directory on failure and propagate errors from
remove_dir_all/rename appropriately so functions referencing skill_dir,
remove_dir_all, clone_status, and source_url are updated accordingly.
In `@clients/agent-runtime/src/skills/sandbox.rs`:
- Around line 126-176: Add a unit test that exercises the
SandboxViolation::SymlinkEscape case by creating a symlink inside the temporary
skill directory that points to a file outside it and asserting
validate_tool_paths returns Err(SandboxViolation::SymlinkEscape { .. }). In the
tests mod add a #[test] #[cfg(unix)] fn (e.g. symlink_escape_rejected) that uses
std::os::unix::fs::symlink to create the link, writes an outside file with
std::fs::write, calls validate_tool_paths(&["your_link_name"],
skill_dir.path()), and asserts matches!(result,
Err(SandboxViolation::SymlinkEscape { .. })). Ensure the test imports fs and
uses tempfile::tempdir() like the other tests.
- Around line 70-76: The traversal check inside the loop over args (for arg in
args) currently only looks for "../", "..\\", or arg == ".." and thus misses
components like "foo/bar/.." — update the check to examine path components
instead of raw substrings: for each arg (in the same loop where
SandboxViolation::TraversalSequence is returned), parse arg as a Path and
iterate its components (or split on both '/' and '\\') and return
SandboxViolation::TraversalSequence when any component equals ".." (or matches
Component::ParentDir) so all trailing or embedded ".." components are caught.
- Around line 105-106: The branch currently allows existing paths when
canonicalize() fails, which weakens security; update the logic around the
path.exists() check and the subsequent canonicalize() call in sandbox.rs so that
if canonicalize() returns an error for an existing path you either reject access
(return an Err) or at minimum log the canonicalization failure with context
before denying; specifically modify the code that calls path.exists() and
path.canonicalize() to treat canonicalize() errors for existing paths as a
security-failing condition (deny-by-default) and surface the error via logging
or a propagated error instead of silently allowing.
In `@clients/agent-runtime/src/skills/scanner.rs`:
- Around line 46-154: The scanner currently loops patterns then lines
(O(patterns×lines)); refactor scan_skill_content to a single-pass loop over
content_lower.lines() and for each line check all pattern sets
(system_override_patterns, role_manipulation_patterns,
trust_escalation_patterns) and the encoded/unicode checks, creating ScanFinding
entries and adding severity as you find matches; keep the existing pattern
arrays and severity values (40, 15, 40) and the EncodedPayload and
UnicodeAnomaly logic but run them per-line inside the same loop to reduce
complexity to O(lines).
In
`@openspec/changes/archive/2026-03-24-official-skills-catalog/archive-report.md`:
- Around line 43-88: The markdown headings "Files Created", "Files Modified",
"Tasks Completed", "Verification Result", "Remaining Warnings", and "Build
Status" are missing surrounding blank lines and trigger MD022; edit
archive-report.md to ensure there's exactly one blank line above and below each
of those headings so each is separated from adjacent paragraphs or lists (e.g.,
add a blank line before the "Files Created" header and after its preceding
content, and similarly for "Files Modified", "Tasks Completed", "Verification
Result", "Remaining Warnings", and "Build Status").
In `@openspec/changes/archive/2026-03-24-official-skills-catalog/exploration.md`:
- Around line 126-131: Add a blank line before the fenced code block under the
bullet describing the new build.rs so the Markdown renders correctly; edit the
section mentioning build.rs / catalog_index.rs and insert an empty line before
the ```rust block that shows pub const EMBEDDED_INDEX and pub const
EMBEDDED_INDEX_COMMIT so the fenced code is separated from the preceding
sentence.
In `@openspec/changes/archive/2026-03-24-official-skills-catalog/tasks.md`:
- Around line 47-52: Task 1.7 in tasks.md is incorrectly marked "pending"
despite the implementation in frontmatter.rs (SkillFrontmatter struct now
containing version: Option<String>, author: Option<String>, tags: Vec<String>
and parse_frontmatter_block() handling "version"/"author"/"tags"); update the
task status to "done" and adjust acceptance note if needed. Open the tasks file
entry for "1.7" and change Status: pending to Status: done, leaving the rest of
the description/acceptance criteria unchanged so it reflects the implemented
SkillFrontmatter and parse_frontmatter_block behavior.
In
`@openspec/changes/archive/2026-03-24-official-skills-catalog/verify-report.md`:
- Around line 221-228: Replace the Update match arm that currently prints a stub
in the SkillCommands::Update match with a call to
handle_update_command(workspace_dir, name.as_deref()) so the implemented update
logic is executed; and in load_skill_md, stop hardcoding version/author/tags and
instead populate the Skill struct from the parsed frontmatter (use
fm.version.unwrap_or_else(|| "0.1.0".to_string()) for version, fm.author for
author, and fm.tags for tags) so the Skill struct’s version, author, and tags
fields reflect the parsed fm values.
In `@openspec/changes/archive/2026-03-24-skills-hardening/design.md`:
- Around line 782-787: Add a migration/release-note entry advising users that
scanner false positives may occur for skills that intentionally include
injection patterns inside code blocks and instruct them to adjust the
skills.scan_threshold setting (or tune RoleManipulation-related patterns)
accordingly; reference the SkillsConfig behavior (lack of deny_unknown_fields
and legacy_open_skills removal) and call out RoleManipulation pattern tuning as
an alternative to code-block-aware parsing so users know where to change
thresholds and why.
In `@openspec/changes/archive/2026-03-24-skills-hardening/proposal.md`:
- Around line 83-98: The pseudocode block in the proposal for A1 (inside
load_skills_with_config()) lacks a language specifier for syntax highlighting;
update the fenced code block to include a language tag (e.g., text or python) so
the lines describing "Read SKILL.md bytes" and "Compute SHA-256 → current_hash"
render with proper highlighting—locate the pseudocode block under the A1.
Content integrity verification section and change the opening triple-backtick to
include the chosen language.
In `@openspec/changes/archive/2026-03-24-skills-hardening/tasks.md`:
- Around line 359-382: Update the tasks.md entry: mark Task 4.4 as "complete"
instead of "pending". Add a unit test in
clients/agent-runtime/src/skills/sandbox.rs that creates a real filesystem
symlink pointing from inside a tempfile sandbox directory to a target outside
that directory, invokes validate_tool_paths() (or the public wrapper used in
tests), and asserts the returned error is SandboxViolation::SymlinkEscape (the
variant referenced at line 94). Ensure the test uses platform-appropriate
symlink creation (or conditional cfg for unix/windows), cleans up temp files,
and follows the same test module style as the existing validate_tool_paths
tests.
In `@openspec/changes/archive/2026-03-24-skills-hardening/verify-report.md`:
- Around line 10-18: The markdown headings "## Completeness" and "### Pending
tasks (per tasks.md)" lack the required surrounding blank lines causing MD022
warnings; fix by adding a blank line before and after each of those heading
lines in verify-report.md so each heading is preceded and followed by an empty
line (ensure "## Completeness" has a blank line above the table and "### Pending
tasks (per tasks.md)" has a blank line above it).
In `@openspec/changes/archive/2026-03-24-skills-product-contracts/exploration.md`:
- Around line 121-126: Change the default for the agent auto-install config so
the system is secure-by-default: set skills.agent_auto_install default value to
"none" instead of "catalog-only", update the code that reads/initializes this
config (look for references to skills.agent_auto_install and any defaults
initialization or parseConfig function) to fallback to "none" when unspecified,
ensure the installer/agent flow still supports "catalog-only" and "none" as
valid options, update docs and the recommendation text to state that
"catalog-only" must be explicitly enabled by the user and add/adjust any tests
that assumed the old default.
In `@openspec/changes/archive/2026-03-24-skills-product-contracts/proposal.md`:
- Around line 9-16: The MD022 lint error is caused by missing blank lines around
the headings "### In Scope" and "### Out of Scope"; update the proposal.md so
each of those heading lines has a blank line before and after them (i.e., ensure
there's an empty line separating the paragraph above from "### In Scope" and
another empty line between the heading and the subsequent list, and do the same
for "### Out of Scope") to satisfy markdownlint heading spacing rules.
In
`@openspec/changes/archive/2026-03-24-skills-product-contracts/specs/official-catalog/spec.md`:
- Around line 96-97: The lockfile `source` prefix in this spec currently shows
`source = "catalog:<name>"` but related docs use the `official:`
prefix—standardize to a single canonical prefix (`official:<name>`) across this
spec and the other archive docs in the PR: update the line in
specs/official-catalog spec (the text showing `source = "catalog:<name>"`) to
`source = "official:<name>"`, then search for and replace any other occurrences
or examples using `catalog:` so all references (examples, prose, and any tests
or sample lockfile snippets) consistently use `official:` to avoid contract
drift.
In `@openspec/changes/archive/2026-03-24-trusted-skills-distribution/design.md`:
- Around line 599-606: The XML emission is inserting unescaped skill metadata
into the prompt (in the write! to prompt using skill.name, skill.description,
and location.display()), so ensure you XML-escape those values before
formatting; locate the write! call that uses skill.trust.as_str(), skill.name,
skill.description, and location.display() and replace each raw value with an
escaped version (e.g., escape & < > " ' into & < > " ') by
calling an existing utility like escape_xml/escape_xml_str or add a small helper
to perform the replacements, then pass the escaped strings into write! to
prevent broken XML or injected prompt content.
- Around line 364-396: build_lock_entry currently serializes only a string
source and lock_entry_to_origin reconstructs only Local or GitRepo, losing
variants like Official, LinkedLocal, and Discovered; update LockEntry to persist
the source variant (e.g., add a source_kind or serialize the SkillSource enum
with its fields) and change build_lock_entry to populate that variant field
(including any associated data such as url, linked path, or discovery metadata),
then update lock_entry_to_origin to match on that persisted variant and
reconstruct the full SkillSource/SkillOrigin (handling Official, LinkedLocal,
Discovered, Local, GitRepo) so round-trip is lossless; also update any lockfile
schema/serialization and unit tests that read/write LockEntry to reflect the new
field names.
- Around line 329-339: The current read_lockfile implementation treats every
std::fs::read_to_string error as an empty lockfile; change it to only treat
ErrorKind::NotFound as the empty case and fail loudly for other I/O errors: in
read_lockfile, after Err(err) from read_to_string, match err.kind() — if
NotFound return SkillsLockfile::default(), otherwise log the full error (e.g.
tracing::error!("failed reading {}: {err}", LOCKFILE_NAME)) and panic or return
an Err (prefer changing the function to return Result<SkillsLockfile,
std::io::Error> if feasible) so permission and I/O failures are not silently
ignored; keep the existing toml::from_str handling and the tracing::warn for
corrupt content.
In
`@openspec/changes/archive/2026-03-24-trusted-skills-distribution/exploration.md`:
- Line 126: Update the prose and example config to use the canonical product
casing "GitHub" instead of "github" where the value is meant as a human-readable
product name; specifically change occurrences in the discussion and examples
that show sources: ["github", "clawhub"] to use "GitHub" (leave the literal
config key lowercase only if the code/config requires it), and ensure the
example command `corvus config skills.discovery.enabled true` remains unchanged.
- Line 7: Replace brittle file:line references like "skills/mod.rs:76-78" with
stable references: use PR links or commit permalinks for the repository snapshot
and/or symbol-level anchors (e.g., refer to the Rust symbol names such as
load_skills() and open_skills_enabled() or skills::load_skills) so readers can
locate the implementation reliably as code moves; update the doc text to state
the exact glob used (`**/*.{md,mdx}`) and replace the other occurrences (lines
noted 15-17 and 336) with the same permalink/symbol approach so all three
references are stable and accurate.
- Around line 3-18: Add a clear, time-scoped banner at the top of the "Current
State" section in exploration.md indicating this is a pre-change snapshot (e.g.,
"State as observed before Phase 1 implementation (March 24, 2026)") so readers
won't confuse it with the post-PR runtime model; update the same clarification
near the later block referenced (around lines 200-213) to maintain consistency;
ensure the banner is concise, unambiguous, and appears before the list that
describes Open-Skills, Workspace skills, and SkillForge behavior.
In
`@openspec/changes/archive/2026-03-24-trusted-skills-distribution/verify-report.md`:
- Around line 49-68: The fenced code blocks in verify-report.md lack language
specifiers; update each triple-backtick block that contains command output
(e.g., the blocks showing "cargo check → Finished dev profile..." and the
Clippy/Format/Test command outputs) to include an appropriate language tag such
as text or shell (for example change ``` to ```text or ```shell) so rendered
output and accessibility are improved; ensure all three blocks in the snippet
are updated consistently.
In `@openspec/specs/local-skills-ux/spec.md`:
- Around line 13-22: The fenced directory-tree code block starting with
"~/.corvus/workspace/skills/" is missing a language specifier; update the
triple-backtick fence to include "text" (e.g., ```text) so the tree is rendered
as plain text (refer to the block containing "~/.corvus/workspace/skills/" and
the subsequent tree lines like "├── my-skill/" and "README.md" and add the
language specifier to the opening fence).
In `@openspec/specs/skill-lifecycle/spec.md`:
- Around line 66-68: The fenced code block containing the command "corvus skills
update [name]" is missing a language specifier; update the Markdown fence to
include the bash language (i.e., change the opening ``` to ```bash) so the block
becomes a bash code block and satisfies static analysis and formatting
consistency for the snippet "corvus skills update [name]".
In `@openspec/specs/third-party-policy/spec.md`:
- Around line 81-85: The spec currently buries the "no re-consent on update"
behavior in Limitations; add a prominent "Known Security Limitations" section
that explicitly calls out that corvus skills update does not re-trigger consent
when a ThirdParty skill changes allowed-tools (allowing silent expansion of
privileges), link or reference an issue tracking re-consent, and recommend
mitigating guidance (e.g., require manual re-consent on updates or note use of
--trust + lockfile as temporary mitigation); update the wording near the
existing "No re-consent on update" bullet and include the unique terms "corvus
skills update", "allowed-tools", "ThirdParty skill", and
"skills.allowed_sources" so readers can locate the related behavior and future
work.
---
Outside diff comments:
In `@clients/agent-runtime/src/skillforge/integrate.rs`:
- Around line 50-97: The generate_md function is inserting unescaped external
metadata (fields on ScoutResult like name, description, owner, url,
language/lang, stars, license and the tags_yaml variable) directly into YAML
frontmatter which can be broken by colons, newlines or YAML markers; fix by
escaping or quoting all frontmatter scalar values before formatting (e.g.,
serialize each scalar with a YAML-safe escaper or use a YAML serializer for the
frontmatter block rather than string interpolation), ensure tags_yaml is
constructed as a safe YAML sequence (escape tag strings or build as a sequence),
and replace direct uses of c.name, c.description, c.owner, c.url, lang, c.stars
and the license expression in generate_md with the escaped/serialized versions
so SKILL.md frontmatter cannot be corrupted.
🪄 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
Run ID: c5e56f7e-16da-4612-b68e-b0133dbd1d26
📒 Files selected for processing (54)
clients/agent-runtime/Cargo.tomlclients/agent-runtime/build.rsclients/agent-runtime/src/agent/prompt.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/config/mod.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/lib.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/onboard/wizard.rsclients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/skillforge/mod.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/catalog_index.tomlclients/agent-runtime/src/skills/frontmatter.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/mod.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/skills/trust.rsclients/agent-runtime/src/skills/validation.rsopenspec/changes/archive/2026-03-24-official-skills-catalog/archive-report.mdopenspec/changes/archive/2026-03-24-official-skills-catalog/design.mdopenspec/changes/archive/2026-03-24-official-skills-catalog/exploration.mdopenspec/changes/archive/2026-03-24-official-skills-catalog/proposal.mdopenspec/changes/archive/2026-03-24-official-skills-catalog/specs/skills-catalog/spec.mdopenspec/changes/archive/2026-03-24-official-skills-catalog/tasks.mdopenspec/changes/archive/2026-03-24-official-skills-catalog/verify-report.mdopenspec/changes/archive/2026-03-24-skills-hardening/ARCHIVE.mdopenspec/changes/archive/2026-03-24-skills-hardening/design.mdopenspec/changes/archive/2026-03-24-skills-hardening/exploration.mdopenspec/changes/archive/2026-03-24-skills-hardening/proposal.mdopenspec/changes/archive/2026-03-24-skills-hardening/specs/skills-hardening/spec.mdopenspec/changes/archive/2026-03-24-skills-hardening/tasks.mdopenspec/changes/archive/2026-03-24-skills-hardening/verify-report.mdopenspec/changes/archive/2026-03-24-skills-product-contracts/design.mdopenspec/changes/archive/2026-03-24-skills-product-contracts/exploration.mdopenspec/changes/archive/2026-03-24-skills-product-contracts/proposal.mdopenspec/changes/archive/2026-03-24-skills-product-contracts/specs/local-skills-ux/spec.mdopenspec/changes/archive/2026-03-24-skills-product-contracts/specs/official-catalog/spec.mdopenspec/changes/archive/2026-03-24-skills-product-contracts/specs/skill-lifecycle/spec.mdopenspec/changes/archive/2026-03-24-skills-product-contracts/specs/third-party-policy/spec.mdopenspec/changes/archive/2026-03-24-skills-product-contracts/tasks.mdopenspec/changes/archive/2026-03-24-trusted-skills-distribution/archive-report.mdopenspec/changes/archive/2026-03-24-trusted-skills-distribution/design.mdopenspec/changes/archive/2026-03-24-trusted-skills-distribution/exploration.mdopenspec/changes/archive/2026-03-24-trusted-skills-distribution/proposal.mdopenspec/changes/archive/2026-03-24-trusted-skills-distribution/specs/skills-trust/spec.mdopenspec/changes/archive/2026-03-24-trusted-skills-distribution/tasks.mdopenspec/changes/archive/2026-03-24-trusted-skills-distribution/verify-report.mdopenspec/specs/local-skills-ux/spec.mdopenspec/specs/official-catalog/spec.mdopenspec/specs/skill-lifecycle/spec.mdopenspec/specs/skills-trust/spec.mdopenspec/specs/third-party-policy/spec.md
Security fixes: - XML-escape skill metadata in prompt rendering (prevents tag injection) - Component-based path traversal detection in sandbox (catches foo/bar/..) - Deny-by-default when canonicalize() fails on existing paths - Atomic update for third-party skills (clone to temp, swap on success) - YAML-escape frontmatter values in SkillForge integrator - Validate catalog_repo_url (https-only, no localhost) and cache TTL - Distinguish NotFound from permission errors in lockfile reader - Add symlink escape test (unix) Bug fixes: - Fix catalog config URL being ignored (map discarded custom value) - Scan all skill prompts, not just first entry Performance: - Single-pass scanner (O(lines) instead of O(patterns x lines)) Refs: #261
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (3)
clients/agent-runtime/src/config/schema.rs (1)
3073-3082:⚠️ Potential issue | 🟠 MajorLoopback validation is still incomplete.
This only blocks exact
localhost,127.0.0.1, and::1; alternate loopback spellings like127.0.0.2or127.1still pass. ReuseSelf::is_loopback_host(host)here so catalog overrides cannot be pointed at local services through other loopback literals.Based on learnings: "Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/config/schema.rs` around lines 3073 - 3082, The current catalog_repo_url host check only matches a few literals and can be bypassed by other loopback addresses; update the validation in the block that reads self.skills.catalog_repo_url to call the existing Self::is_loopback_host(host) helper instead of the manual comparisons (or add it as an additional condition) so any loopback form (e.g., 127.0.0.2, 127.1, ::1 variants) is rejected; ensure you still treat empty host as invalid and keep the https scheme check and the same error messages but use Self::is_loopback_host(host) to detect loopback addresses.clients/agent-runtime/src/skills/mod.rs (2)
367-393:⚠️ Potential issue | 🟠 MajorCLI skill commands still ignore configured catalog settings.
These handlers either rebuild
SkillsConfig::default()or hard-codeOFFICIAL_REPO/scanner::DEFAULT_SCAN_THRESHOLD, socatalog_repo_url, cache TTL, and the configured scan threshold never affectlist --catalog,search,install, or officialupdate. Pass&SkillsConfigthrough the CLI boundary once and use it everywhere below.As per coding guidelines, "Security first, performance second. Validate input boundaries, auth/authz implications, and secret management. Look for behavioral regressions, missing tests, and contract breaks across modules."
Also applies to: 407-409, 509-511, 695-696, 743-743, 876-904, 1170-1171, 1205-1205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/skills/mod.rs` around lines 367 - 393, The CLI handlers are currently reconstructing SkillsConfig via SkillsConfig::default() or using hard-coded OFFICIAL_REPO and scanner::DEFAULT_SCAN_THRESHOLD so configured catalog_repo_url, cache TTL, and scan threshold are ignored; change handle_command to accept a &SkillsConfig (or obtain it once before matching) and pass that &SkillsConfig into downstream functions (handle_list_catalog, handle_list_command, handle_install_command, handle_search_command, handle_update_command, handle_discover_command, and handle_lock_repair_command if needed) and replace any use of SkillsConfig::default(), OFFICIAL_REPO, or scanner::DEFAULT_SCAN_THRESHOLD inside those callsites with values read from the passed SkillsConfig so all catalog, cache, and scan threshold settings are honored end-to-end.
1201-1238:⚠️ Potential issue | 🟠 MajorKeep the current official skill until the replacement is ready.
Line 1202 deletes the installed skill before clone/copy/validation succeeds. Any later failure leaves the user with nothing installed. Mirror the temp-dir swap used in
update_thirdparty_skill()so the old version survives until the new tree is fully staged.Safer update flow
- if skill_dir.exists() { - std::fs::remove_dir_all(&skill_dir)?; - } - let repo_url = catalog::OFFICIAL_REPO; let temp_base = std::env::temp_dir().join(format!("corvus-update-{name}")); let _ = std::fs::remove_dir_all(&temp_base); @@ - copy_dir_recursive(&source_path, &skill_dir)?; - let _ = std::fs::remove_dir_all(&temp_base); + let staged_dir = skill_dir.with_extension("tmp"); + let _ = std::fs::remove_dir_all(&staged_dir); + copy_dir_recursive(&source_path, &staged_dir)?; + let _ = std::fs::remove_dir_all(&temp_base); + + if skill_dir.exists() { + std::fs::remove_dir_all(&skill_dir)?; + } + std::fs::rename(&staged_dir, &skill_dir)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/skills/mod.rs` around lines 1201 - 1238, The current flow removes the installed skill up front (skill_dir) which can leave users without a skill if clone/copy/validation fails; change it to mirror update_thirdparty_skill(): do NOT call std::fs::remove_dir_all(&skill_dir) at the start, instead clone the repo into temp_base, verify source_path exists and copy or stage the new tree into a temporary staging directory (using copy_dir_recursive or equivalent), then atomically remove or rename the old skill_dir and move/rename the staged directory into place only after all operations (clone, validation, copy) succeed; use the same temporary swap/rename semantics as update_thirdparty_skill() and still clean up temp_base on both success and failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/skillforge/integrate.rs`:
- Around line 172-181: The test currently only checks SKILL.md text via
contains() — update it to parse the YAML frontmatter and assert the parsed
struct fields to validate quoting/escaping round-trip: read SKILL.md into md,
extract the frontmatter block and pass it through the same frontmatter
parser/loader used in your skills module (the loader in skills::mod or its
parse_frontmatter/load_skill_frontmatter function), then assert that name ==
"test-skill", description == "A test skill for unit tests" (add a variant test
with quotes in the description to ensure escaping is preserved), version ==
"0.1.0", author == "user", and tags contains "discovered" and "Rust" rather than
using raw contains() on the file text.
- Around line 52-60: The frontmatter writer must stop emitting backslash-escaped
quoted scalars (which yaml_escape produces) because
clients/agent-runtime/src/skills/frontmatter.rs only strips outer quotes and
never unescapes; update the code that builds tags_yaml and the format! block
(the variables/expressions that fill name, description, owner and lang in
integrate.rs) to serialize only the subset the parser understands: sanitize each
field by removing newlines, replacing internal double-quotes with a safe
character (or single quotes), and emit plain unquoted scalars (and ensure the
tags list uses the sanitized lang value), or alternatively wire up a writer that
matches the frontmatter parser—do not emit backslash escapes. Ensure the same
change is applied to the other writer sites mentioned (the blocks at the other
occurrences: ~lines 88-90 and 104-110).
- Around line 79-80: Replace the current install example "corvus skills install
{raw_name}" in integrate.rs with a URL-based example showing how to install
discovered third-party skills (e.g., "corvus skills install
https://github.com/owner/skill-name") and include the required trust flag (e.g.,
append "--trust") so the documentation demonstrates installing
external/discovered skills rather than only catalog names; update the line
containing the raw_name example accordingly.
In `@clients/agent-runtime/src/skills/catalog.rs`:
- Around line 55-65: After parsing the TOML into CatalogIndex in parse_index,
validate every CatalogEntry.path: use Path::new(&entry.path) and reject any path
that is absolute (is_absolute()) or contains parent-directory components (scan
components for Component::ParentDir) or other non-normal components
(Prefix/RootDir); if any invalid path is found, return an error (anyhow::bail)
explaining which entry/path failed so the index is not accepted. Add this check
in parse_index after the version check, iterating index.entries and referencing
CatalogEntry.path to locate offending entries.
In `@clients/agent-runtime/src/skills/lockfile.rs`:
- Around line 55-60: The write/update paths currently using std::fs::write
(e.g., write_lock_entry which calls read_lockfile and writes LOCKFILE_NAME, and
the other places flagged that perform direct writes) must write skills.lock
atomically: create a temporary file in the same directory, serialize the updated
Lockfile to the temp file, fsync the temp file and its parent directory, then
atomically rename the temp file to LOCKFILE_NAME (mirroring the approach used by
Config::save() in Config schema). Replace direct std::fs::write calls with this
temp-file+fsync+rename sequence for write_lock_entry and the other write/remove
functions so an interrupted update cannot leave a truncated/corrupt skills.lock.
- Around line 158-182: The repair_lockfile() logic weakens security by dropping
an existing content_hash when std::fs::read(&skill_md).ok() fails and by
creating new LockEntry with trust "local"; change it so read failures do not
overwrite an existing entry's content_hash (i.e., compute current_hash as Option
and when existing is Some(&mut existing) keep existing.content_hash unchanged on
read error instead of setting it to None) and when creating a new LockEntry
default the trust field to the most restrictive tier (use the project's
restrictive token, e.g., "restricted"/"third-party"/"sandboxed" consistent with
clients/agent-runtime/src/skills/mod.rs) rather than "local", leaving
source/path/pinned_ref behavior unchanged.
In `@clients/agent-runtime/src/skills/mod.rs`:
- Around line 1271-1343: The update_thirdparty_skill function currently clones
into temp_dir and immediately swaps/updates the lockfile; modify it to run the
same install validation pipeline against the temp checkout (validate SKILL.md
presence, parse and verify frontmatter/name consistency, compute and compare
content hash, and run the prompt-injection/security scan and trust-escalation
logic used by third-party installs) before performing the atomic swap and
writing the new lockfile; if validation fails, remove temp_dir and propagate the
same trust-threshold response (reject or escalate) rather than renaming or
updating the lockfile, and ensure all error paths clean up temp_dir and preserve
existing skill_dir.
- Around line 57-64: The wrapper API currently forces verify_integrity=true and
the bool-based load_skills_with_config cannot carry scan_threshold or future
policy fields; replace the bool shim with a SkillsConfig struct and thread it
through: change load_skills(workspace_dir: &Path) to call
load_skills_with_config(workspace_dir, config::SkillsConfig::default()) (or
better, accept a SkillsConfig parameter), update the signature of
load_skills_with_config to accept SkillsConfig instead of bool, propagate the
new SkillsConfig parameter into the internal loading logic, and update all call
sites (notably the callers in clients/agent-runtime/src/agent/agent.rs and
clients/agent-runtime/src/channels/mod.rs) to pass config.skills so
config.skills.verify_integrity and scan_threshold are honored rather than being
silently ignored.
In `@clients/agent-runtime/src/skills/sandbox.rs`:
- Around line 88-135: The code currently checks path.exists() which follows
symlinks and lets dangling symlinks bypass the sandbox; update the logic in the
function handling path checks so you call path.symlink_metadata() (or
std::fs::read_link when appropriate) before path.exists() to detect symlinks and
dangling symlinks, and reject them by returning SandboxViolation::SymlinkEscape
(use arg.to_string() for path and the resolved target or canonical_skill for
target) instead of falling through to the "path doesn't exist yet" branch;
preserve the existing canonicalization behavior for non-symlink paths (use
skill_dir.canonicalize().unwrap_or_else(|_| skill_dir.to_path_buf()) as
canonical_skill) and keep the PathEscape responses for true path escapes
detected after canonicalization.
In `@clients/agent-runtime/src/skills/scanner.rs`:
- Around line 123-137: The current per-line check using trimmed.len() only
catches single-line base64 blobs; update the logic in scanner.rs (around the
block that uses trimmed, findings, ScanFinding, ScanCategory::EncodedPayload,
and score) to detect contiguous base64-like lines: when a line passes the
per-line base64 char test, accumulate it and subsequent adjacent lines that also
satisfy the same byte test into a single buffer (stop when a line fails the test
or is separated by blank/non-base64), then evaluate the combined length and, if
>200, push one ScanFinding with category EncodedPayload and pattern indicating
the combined block length and add 30 to score once; ensure you preserve existing
behavior for single-line blobs and avoid duplicating findings for each wrapped
line.
---
Duplicate comments:
In `@clients/agent-runtime/src/config/schema.rs`:
- Around line 3073-3082: The current catalog_repo_url host check only matches a
few literals and can be bypassed by other loopback addresses; update the
validation in the block that reads self.skills.catalog_repo_url to call the
existing Self::is_loopback_host(host) helper instead of the manual comparisons
(or add it as an additional condition) so any loopback form (e.g., 127.0.0.2,
127.1, ::1 variants) is rejected; ensure you still treat empty host as invalid
and keep the https scheme check and the same error messages but use
Self::is_loopback_host(host) to detect loopback addresses.
In `@clients/agent-runtime/src/skills/mod.rs`:
- Around line 367-393: The CLI handlers are currently reconstructing
SkillsConfig via SkillsConfig::default() or using hard-coded OFFICIAL_REPO and
scanner::DEFAULT_SCAN_THRESHOLD so configured catalog_repo_url, cache TTL, and
scan threshold are ignored; change handle_command to accept a &SkillsConfig (or
obtain it once before matching) and pass that &SkillsConfig into downstream
functions (handle_list_catalog, handle_list_command, handle_install_command,
handle_search_command, handle_update_command, handle_discover_command, and
handle_lock_repair_command if needed) and replace any use of
SkillsConfig::default(), OFFICIAL_REPO, or scanner::DEFAULT_SCAN_THRESHOLD
inside those callsites with values read from the passed SkillsConfig so all
catalog, cache, and scan threshold settings are honored end-to-end.
- Around line 1201-1238: The current flow removes the installed skill up front
(skill_dir) which can leave users without a skill if clone/copy/validation
fails; change it to mirror update_thirdparty_skill(): do NOT call
std::fs::remove_dir_all(&skill_dir) at the start, instead clone the repo into
temp_base, verify source_path exists and copy or stage the new tree into a
temporary staging directory (using copy_dir_recursive or equivalent), then
atomically remove or rename the old skill_dir and move/rename the staged
directory into place only after all operations (clone, validation, copy)
succeed; use the same temporary swap/rename semantics as
update_thirdparty_skill() and still clean up temp_base on both success and
failure.
🪄 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
Run ID: 9585e7c7-a3db-4e99-a001-7bf5b5af536a
📒 Files selected for processing (9)
clients/agent-runtime/src/agent/prompt.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/lib.rsclients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/mod.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/scanner.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pr-checks
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (5)
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/agent/prompt.rsclients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/lib.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/mod.rs
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/src/agent/prompt.rsclients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/lib.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/mod.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/src/agent/prompt.rsclients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/lib.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/mod.rs
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/agent-runtime/src/agent/prompt.rsclients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/lib.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/mod.rs
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Files:
clients/agent-runtime/src/config/schema.rs
🧠 Learnings (8)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Keep startup path lean and avoid heavy initialization in command parsing flow
Applied to files:
clients/agent-runtime/src/agent/prompt.rsclients/agent-runtime/src/skills/catalog.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Applied to files:
clients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/lib.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/tools/**/*.rs : Implement `Tool` trait in `src/tools/` with strict parameter schema, validate and sanitize all inputs, and return structured `ToolResult` without panics in runtime path
Applied to files:
clients/agent-runtime/src/lib.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Applied to files:
clients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Applied to files:
clients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/**/*.rs : Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Applied to files:
clients/agent-runtime/src/skills/catalog.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/Cargo.toml : Do not add heavy dependencies for minor convenience; justify new crate additions
Applied to files:
clients/agent-runtime/src/skills/mod.rs
Security fixes: - Use is_loopback_host helper for catalog URL validation (catches 127.x variants) - Validate catalog index entry paths (reject absolute/traversal paths) - Atomic lockfile writes (temp file + rename to prevent corruption) - Default unknown skills in repair to third-party trust (not local) - Preserve existing hash on repair read failure (don't weaken integrity) - Detect dangling symlinks in sandbox (symlink_metadata before exists) - Thread SkillsConfig through load_skills and handle_command (honor config) - Atomic update for official skills (staging dir + swap) - Validate + scan third-party skills after update - Fix yaml_sanitize to match frontmatter parser (no backslash escapes) - Multi-line base64 detection in scanner - Scan all skill prompts, not just first entry Bug fixes: - Install example in SkillForge uses URL + --trust (not bare name) - Frontmatter test validates parsed struct fields (round-trip) Refs: #261
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
clients/agent-runtime/src/skillforge/integrate.rs (1)
51-53:⚠️ Potential issue | 🟠 MajorSanitize
languagebefore writing it intotags:.Line 52 still interpolates raw
langinto the frontmatter list. A value containing a newline,:, or#can still breakparse_frontmatter()or inject extra frontmatter keys even though the scalar fields are sanitized.🔧 Suggested fix
fn generate_md(&self, c: &ScoutResult) -> String { let lang = c.language.as_deref().unwrap_or("unknown"); - let tags_yaml = format!(" - discovered\n - {lang}"); + let lang_tag = yaml_sanitize(lang); + let tags_yaml = format!(" - discovered\n - {lang_tag}");Also applies to: 59-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/skillforge/integrate.rs` around lines 51 - 53, The frontmatter tag list is built from raw c.language (variable lang) and can contain newlines or special chars that break parse_frontmatter(); sanitize or escape lang before embedding it into tags_yaml (e.g., reject/replace newlines, ':' and '#' or percent-escape/JSON-quote the value) and then use the sanitized value when building tags_yaml; apply the same sanitization to the other occurrence that builds tags (the second tags_yaml usage near the same block) so parse_frontmatter() cannot be broken by injected characters.clients/agent-runtime/src/skills/sandbox.rs (1)
88-118:⚠️ Potential issue | 🔴 CriticalSymlinked parent directories still let new files escape the sandbox.
symlink_metadata()andexists()are only checking the full target path. Forescape-dir/new.txt, whereescape-diris a symlink insideskill_dirto/tmp/outside, these branches returnOk(())becausenew.txtdoes not exist yet; the later create/write then follows the parent symlink outside the sandbox. Walk each existing ancestor withsymlink_metadata()/canonicalize()before allowing a non-existent leaf, and add a regression test for that case.Also applies to: 153-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/skills/sandbox.rs` around lines 88 - 118, The current symlink check only inspects the final path component and misses symlinked parent directories (allowing creation in escaped locations); update the logic that handles PathBuf path (the block using path.symlink_metadata(), path.canonicalize(), skill_dir.canonicalize() and returning SandboxViolation::SymlinkEscape) to walk each existing ancestor of the requested path (using symlink_metadata()/canonicalize() on parents up to skill_dir) and verify each canonicalized ancestor remains inside the canonicalized skill_dir before permitting a non-existent leaf, returning SandboxViolation::SymlinkEscape with the same fields (path/target/skill_dir) on any violation; also add a regression test covering the case where a symlinked directory inside the sandbox points outside (e.g., escape-dir/new.txt where escape-dir is a symlink to /tmp/outside) to ensure new file creation is denied.clients/agent-runtime/src/skills/lockfile.rs (2)
39-50:⚠️ Potential issue | 🟠 MajorDon't treat an unreadable
skills.lockas missing.Line 49 collapses
PermissionDeniedand transient I/O failures intoSkillsLockfile::default(), so loaders proceed without trust, allowed-tools, or integrity metadata for every skill. Keep the empty fallback forNotFoundonly; other read failures should be surfaced or handled as a fail-closed condition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/skills/lockfile.rs` around lines 39 - 50, The read_lockfile function currently treats any read_to_string error as a missing lockfile; change read_lockfile to surface non-NotFound I/O errors instead of silently returning SkillsLockfile::default(): when std::fs::read_to_string(&path) returns Err(err), match on err.kind() and return SkillsLockfile::default() only for std::io::ErrorKind::NotFound, but propagate or return Err(err) for other kinds (e.g., PermissionDenied) — this will require updating read_lockfile's signature to return Result<SkillsLockfile, std::io::Error> (or another suitable error type) and adjusting callers; keep the existing TOML parse handling in read_lockfile (toml::from_str -> warn + default) or similarly wrap parse errors in the Result if you prefer failing closed on corrupt content.
107-121:⚠️ Potential issue | 🔴 CriticalRepaired and unknown entries still downgrade to
Localtrust.
lock_entry_to_origin()derives the runtime trust solely fromsource. Line 188 writes repaired entries assource = "local", and Line 120 also maps unknownsourcestrings toSkillSource::Local;clients/agent-runtime/src/skills/trust.rs:71-80then converts that toSkillTrust::Local, so the next load loses third-party sandboxing despite Line 187 storingtrust = "third-party". Encode a third-party source here or make unknown/mismatched sources fail closed.Also applies to: 185-188
clients/agent-runtime/src/skills/mod.rs (1)
1360-1378:⚠️ Potential issue | 🟠 MajorThird-party updates bypass trust gate on scanner threshold breach.
When updating a third-party skill, the scanner runs (line 1368) but only logs a warning if the threshold is exceeded. Unlike install-time behavior, there's no abort or trust confirmation. A malicious update could inject content that would have been blocked at install time.
Consider either:
- Rejecting updates that exceed the threshold (requiring
--forceor similar)- Clearing
allowed_toolsin the new lockfile entry when threshold is exceeded🛠️ Option 2: Disable tools on threshold breach
// Run scanner on updated content if let Ok(content) = std::fs::read_to_string(&skill_md) { let scan = scanner::scan_skill_content(&content); if scan.exceeds_threshold(scanner::DEFAULT_SCAN_THRESHOLD) { tracing::warn!( "updated skill '{}' scored {} in injection scan \ (threshold: {}). Review the skill content.", name, scan.score, scanner::DEFAULT_SCAN_THRESHOLD, ); + // Disable tools for this update - matches install-time behavior + entry_allowed_tools = None; } } // Update lockfile ... let new_entry = lockfile::build_lock_entry( trust::SkillTrust::ThirdParty, source_url, None, hash, - entry.allowed_tools.clone(), + entry_allowed_tools, None, );
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/skills/mod.rs`:
- Around line 898-929: The install-time scanner uses
scanner::DEFAULT_SCAN_THRESHOLD but should use the user-configured threshold
(config.scan_threshold) like load-time scanning; update the two places where
scanner::DEFAULT_SCAN_THRESHOLD is referenced in the SKILL.md scan block to use
config.scan_threshold (or otherwise obtain the same RuntimeConfig/Settings used
at load time), and if config isn't in scope, add it to the function signature or
fetch it from the same config source so the logic that calls
scan.exceeds_threshold(...) and the tracing::warn! message both use the
consistent config.scan_threshold value; keep the rest of the flow (abort on
exceeds without trust_flag, warn-and-proceed with trust_flag) unchanged.
---
Duplicate comments:
In `@clients/agent-runtime/src/skillforge/integrate.rs`:
- Around line 51-53: The frontmatter tag list is built from raw c.language
(variable lang) and can contain newlines or special chars that break
parse_frontmatter(); sanitize or escape lang before embedding it into tags_yaml
(e.g., reject/replace newlines, ':' and '#' or percent-escape/JSON-quote the
value) and then use the sanitized value when building tags_yaml; apply the same
sanitization to the other occurrence that builds tags (the second tags_yaml
usage near the same block) so parse_frontmatter() cannot be broken by injected
characters.
In `@clients/agent-runtime/src/skills/lockfile.rs`:
- Around line 39-50: The read_lockfile function currently treats any
read_to_string error as a missing lockfile; change read_lockfile to surface
non-NotFound I/O errors instead of silently returning SkillsLockfile::default():
when std::fs::read_to_string(&path) returns Err(err), match on err.kind() and
return SkillsLockfile::default() only for std::io::ErrorKind::NotFound, but
propagate or return Err(err) for other kinds (e.g., PermissionDenied) — this
will require updating read_lockfile's signature to return Result<SkillsLockfile,
std::io::Error> (or another suitable error type) and adjusting callers; keep the
existing TOML parse handling in read_lockfile (toml::from_str -> warn + default)
or similarly wrap parse errors in the Result if you prefer failing closed on
corrupt content.
In `@clients/agent-runtime/src/skills/sandbox.rs`:
- Around line 88-118: The current symlink check only inspects the final path
component and misses symlinked parent directories (allowing creation in escaped
locations); update the logic that handles PathBuf path (the block using
path.symlink_metadata(), path.canonicalize(), skill_dir.canonicalize() and
returning SandboxViolation::SymlinkEscape) to walk each existing ancestor of the
requested path (using symlink_metadata()/canonicalize() on parents up to
skill_dir) and verify each canonicalized ancestor remains inside the
canonicalized skill_dir before permitting a non-existent leaf, returning
SandboxViolation::SymlinkEscape with the same fields (path/target/skill_dir) on
any violation; also add a regression test covering the case where a symlinked
directory inside the sandbox points outside (e.g., escape-dir/new.txt where
escape-dir is a symlink to /tmp/outside) to ensure new file creation is denied.
🪄 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
Run ID: 1794fd6f-0893-45b7-b135-f7d11fd67a1c
📒 Files selected for processing (8)
clients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/mod.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/scanner.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pr-checks
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (6)
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/mod.rs
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/mod.rs
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Files:
clients/agent-runtime/src/config/schema.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/mod.rs
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/mod.rs
clients/agent-runtime/src/main.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/main.rs: Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Keep startup path lean and avoid heavy initialization in command parsing flow
Files:
clients/agent-runtime/src/main.rs
🧠 Learnings (9)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Applied to files:
clients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/skills/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Applied to files:
clients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Applied to files:
clients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/tools/**/*.rs : Implement `Tool` trait in `src/tools/` with strict parameter schema, validate and sanitize all inputs, and return structured `ToolResult` without panics in runtime path
Applied to files:
clients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/skills/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/src/main.rsclients/agent-runtime/src/skills/sandbox.rsclients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skillforge/integrate.rsclients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/Cargo.toml : Do not add heavy dependencies for minor convenience; justify new crate additions
Applied to files:
clients/agent-runtime/src/skills/lockfile.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/**/*.rs : Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Applied to files:
clients/agent-runtime/src/skills/lockfile.rsclients/agent-runtime/src/skills/scanner.rsclients/agent-runtime/src/skills/catalog.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Keep startup path lean and avoid heavy initialization in command parsing flow
Applied to files:
clients/agent-runtime/src/skills/catalog.rsclients/agent-runtime/src/skills/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Include threat/risk notes and rollback strategy for security, runtime, and gateway changes; add or update tests for boundary checks and failure modes
Applied to files:
clients/agent-runtime/src/skills/mod.rs
🔇 Additional comments (14)
clients/agent-runtime/src/skills/catalog.rs (4)
156-191: Blocking HTTP client in async call chain.
try_fetch_indexusesreqwest::blocking::Clientbut is invoked through the asynchandle_cli_commandpath. While the 3-second timeout bounds the worst case, this still blocks the tokio runtime thread. Consider wrapping intokio::task::spawn_blockingor converting to asyncreqwest::Client.
55-85: LGTM — Path traversal validation is sound.The
parse_indexfunction correctly rejects absolute paths and..components, preventing directory traversal attacks from malicious catalog entries.
117-120: LGTM — Custom catalog URL is now honored.The previous bug that discarded custom
catalog_repo_urlvalues has been fixed.
200-214: LGTM — Search implementation is clean.Case-insensitive matching across name, description, and tags is efficient for the expected catalog size.
clients/agent-runtime/src/main.rs (2)
471-517: LGTM — CLI schema expansion is well-structured.New subcommands (
Search,Update,Discover,Lock) and the--trustflag onInstallfollow the established pattern. The--catalogflag onListprovides clear discoverability.
788-790: LGTM — Config threading is correct.
skills::handle_commandnow receives&config.skills, enabling proper propagation ofverify_integrity,scan_threshold, and catalog settings.clients/agent-runtime/src/skills/scanner.rs (2)
45-194: LGTM — Scanner implementation is solid.Single-pass pattern matching plus a dedicated multi-line base64 detection pass is an acceptable tradeoff for typical skill file sizes. The severity weights and threshold semantics (score > threshold, not >=) are well-tested.
151-191: Multi-line base64 detection addresses the bypass concern.Contiguous base64-like lines are now aggregated and scored as a single block when their combined length exceeds 200 chars.
clients/agent-runtime/src/skills/mod.rs (6)
57-155: LGTM — Skill loading pipeline with integrity and scan checks.The load flow correctly:
- Enriches skills from lockfile (trust/origin/allowed_tools)
- Verifies integrity with trust-tier-aware handling (ThirdParty clears tools on mismatch)
- Scans ThirdParty skills against configurable threshold
- Sets
sandboxedflag on tools based on trust tier
127-146: Scanner now checks all prompts.The
skill.prompts.join("\n")ensures the entire skill content is scanned, addressing the previous concern about only checking the first entry.
239-259: LGTM — Trust-based tool filtering is correct.Official/Local skills expose all tools; ThirdParty skills only expose tools in
allowed_tools, defaulting to none (instruction-only mode).
300-313: LGTM — Sandbox check delegates to validation module.The guard correctly skips non-sandboxed tools and ensures sandboxed tools have a valid skill directory context.
1192-1303: LGTM — Official skill update uses atomic staging + swap.The temp clone → staging dir → swap pattern prevents data loss if the clone fails.
1325-1358: LGTM — Third-party update uses atomic temp → swap.Clone to temp, remove .git, then atomic rename prevents partial updates.
- Use config.scan_threshold instead of DEFAULT_SCAN_THRESHOLD in install flow - Sanitize language field in SkillForge frontmatter tags (yaml_sanitize) - Restore NotFound distinction in lockfile reader (log non-NotFound errors) - Walk ancestor directories in sandbox to catch symlinked parent escapes - Add symlinked_parent_directory_escape_rejected test Refs: #261
Add comprehensive tests for previously uncovered pure functions and integration paths in the skills trust implementation: mod.rs: resolve_skill_source, extract_description, filter_tools_by_trust, validate_skill_name_path_safety, ensure_skill_path_stays_within_root, copy_dir_recursive, format_tool_names, check_sandbox, handle_remove, skills_to_prompt ThirdParty filtering, load_skill_md with frontmatter, lockfile enrichment integration, is_remote_skill_source edge cases prompt.rs: escape_xml (special chars, no special, all special), render_skills_section trust sorting and attribute verification catalog.rs: search by description/tag, is_bare_name edge cases, resolve_index embedded fallback scanner.rs: combined patterns threshold, empty content, normal content sandbox.rs: embedded traversal components, relative subdir, empty args Refs: #261
|
yacosta738
deleted the
feature/261-define-trusted-skills-distribution-and-installation-model-for-corvus
branch
Merged
This was referenced Apr 9, 2026
Merged
This was referenced Apr 10, 2026
Merged
Merged
This was referenced Apr 19, 2026
Closed
This was referenced Apr 28, 2026
Merged
8 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces several improvements and refactors to the Corvus agent runtime, focusing on skill management, configuration, and catalog integration. The most significant changes include the addition of a dedicated skills configuration section, enhancements to the CLI skill commands, improved rendering and safety information for skills in prompts, and a refactor of the SkillForge integration process to generate only a single manifest file per skill.
Skill management and configuration improvements:
Added a new
SkillsConfigsection to the main configuration (Config) for controlling catalog repository URL, cache TTL, integrity verification, and prompt injection scan threshold. This is now included in all configuration creation paths and defaults. [1] [2] [3] [4] [5] [6] [7] [8]Added a build script (
build.rs) to embed the catalog index snapshot for offline-first skill discovery, improving reliability and startup performance. [1] [2]CLI and skill command enhancements:
SkillCommandsCLI interface to support catalog listing, installation with trust acknowledgment, searching, updating, discovering third-party skills, and lockfile maintenance. Also added subcommands for lockfile repair. [1] [2]Skill prompt and rendering improvements:
SkillForge integration refactor:
Refactored SkillForge integration to generate only a
SKILL.mdwith YAML frontmatter (instead of bothSKILL.tomlandSKILL.md). Updated tests to reflect this change and removed TOML escaping logic. [1] [2] [3] [4] [5] [6] [7]Deprecated and disabled the
auto_integrateoption inSkillForgeConfig, warning users to use the new CLI-based workflow instead. [1] [2]