fix(skills): stop flagging remote markdown documentation links in skill audit

[IftekharUddin]

Summary

• Base branch: master
• What changed and why:
  • The skill audit's audit_markdown_link_target flagged any http/https/mailto markdown link whose target ended in .md/.markdown with remote markdown links are blocked. This is a false positive — the loader never fetches those URLs at audit/load time; they pass through as prompt text, and any later fetch stays gated by the existing tool/network policy.
  • A .md suffix does not reliably indicate Markdown (e.g. Cloudinary serves HTML at .md routes), so the check carried no load-bearing security while silently downgrading or skipping real marketplace skills (cloudinary-transformations cites ~40 such docs links; Sanity skills similarly).
  • Removed only that inner branch so the http/https/mailto arm early-returns with no finding. The early return is kept deliberately — dropping the whole arm would route these links into the unsupported URL scheme finding instead.
  • Added two regression tests: remote .md/.markdown/mailto links audit clean, and unsupported schemes (javascript:/file:) are still rejected.
• Scope boundary: No new config knobs or audit options; no change to cross-skill / open-skills reference logic; no change to shell or network policy. Single file: crates/zeroclaw-runtime/src/skills/audit.rs.
• Blast radius: Skill audit only. Skills that previously failed audit solely for citing .md doc URLs now load; every other audit finding is unchanged — unsupported URL schemes, absolute/unsafe local paths, script-suffix links, and broken same-skill-root references all still fire.
• Linked issue(s): Closes [Feature]: Remove remote-markdown-link block from skill audit #6714
• Labels: (snapshot after maintainer applies; see note below)

Validation Evidence (required)

• Commands run and tail output:

  cargo fmt --all -- --check → exit 0 (no diff).

  cargo clippy --all-targets -- -D warnings → exit 0, no warnings (all crates):

      Checking zeroclaw-runtime v0.8.1 (.../crates/zeroclaw-runtime)
      Checking zeroclaw-channels v0.8.1 (.../crates/zeroclaw-channels)
      Checking zeroclaw-eval v0.8.1 (.../crates/zeroclaw-eval)
      Finished `dev` profile [unoptimized + debuginfo] target(s) in 2m 35s
  

  cargo test (root crate, mirrors dev/ci.sh's cargo test --locked) → 903 passed, 0 failed:

  test result: ok. 236 passed; 0 failed; ...   (zeroclaw lib)
  test result: ok. 289 passed; 0 failed; ...   (zeroclaw main)
  test result: ok. 188 passed; 0 failed; ...   (test_component)
  test result: ok. 159 passed; 0 failed; ...   (test_integration)
  test result: ok. 9 passed; 0 failed; ...     (test_system)
  

  cargo test -p zeroclaw-runtime --lib (the changed crate — the root cargo test above does not reach it) → 2347 passed, 0 failed, including the two new tests:

  test skills::audit::tests::audit_allows_remote_markdown_documentation_links ... ok
  test skills::audit::tests::audit_still_rejects_unsupported_url_schemes ... ok
  test result: ok. 2347 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out
  

  cargo test --workspace --locked (every member crate, comprehensive) → 9456 passed, 0 failed across all 67 test binaries; both new audit tests included.

• Beyond CI — what I manually verified: Confirmed has_markdown_suffix is still referenced elsewhere (no dead code); confirmed the markdown-link regex still captures javascript:/file:// targets so the unsupported-scheme check keeps firing (the new test asserts exactly two such findings); confirmed via grep that no other skills load/audit path fetches remote markdown — the only reqwest use under skills/ downloads the install bundle, which is unrelated. Not verified: live end-to-end install of a Cloudinary/Sanity skill (no network/marketplace access in this environment); the audit-level behavior is covered by the unit tests above.

• If any command was intentionally skipped, why: None skipped.

Security & Privacy Impact (required)

• New permissions, capabilities, or file system access scope? No
• New external network calls? No — this removes audit-time scrutiny of links the runtime never fetches; any downstream fetch remains gated by the existing tool/network policy.
• Secrets / tokens / credentials handling changed? No
• PII, real identities, or personal data in diff, tests, fixtures, or docs? No — tests use example.com and a mailto:support@example.com placeholder.

Context: although this touches the security audit, the removed check provided no load-bearing security (no load-time fetch occurs, and a .md suffix is not an authoritative content-type signal). All structural audit guarantees are preserved.

Compatibility (required)

• Backward compatible? Yes
• Config / env / CLI surface changed? No
• Upgrade steps for existing users: None. Skills that previously failed audit only because they cited .md documentation URLs now load successfully; no action required.

Rollback

Low risk: git revert <merge sha> fully restores prior behavior.

• Fast rollback command/path: git revert <merge sha>
• Feature flags or config toggles: None
• Observable failure symptoms: the remote markdown links are blocked by skill security audit (<url>) finding reappearing in skill audit output / zeroclaw skills list, or skills with .md doc links being downgraded/skipped again.

---

Note on labels: issue #6714 carries risk: high, but this change removes a single conditional branch with no cross-crate blast radius; suggested risk: low, size: S, scope skills/runtime/security. Deferring to maintainers for the final label set.

[Audacity88]

Thanks @IftekharUddin. I reviewed the live PR state at 6b704db, the linked #6714 acceptance comments, the one-file skills/audit.rs diff, the surrounding audit and skill-loading code, and the public CI/check status. I did not run local Cargo for this review-only pass; I relied on the PR's reported validation, green public CI, and source review.

🟢 What looks good — The cleanup preserves the skill-audit boundary

This matches the policy boundary accepted in #6714. The audit still handles local structure: unsupported URL schemes continue to be rejected, unsafe local paths and script-like link targets still route through the existing checks, and same-skill-root Markdown references still get resolved and validated. The change removes only the false-positive branch for remote http / https / mailto documentation links that happen to end in .md or .markdown.

The regression coverage is also pointed at the right risks. One test proves remote documentation links now audit clean, and the other proves javascript: and file: Markdown links still produce unsupported-scheme findings. I also checked the surrounding skill-loading path: these Markdown links are read as prompt text, not fetched by the audit or loader, so later network access remains a tool-policy decision rather than a load-time side effect.

Approving — the implementation preserves the structural audit boundary accepted in #6714, the regression tests cover the right risks, and all visible CI checks pass.

[WareWolf-MoonWall]

Review — PR #8190 fix(skills): stop flagging remote markdown documentation links in skill audit

Author: IftekharUddin
Head SHA: 6b704db
Verdict: --approve
CI: 18/18 passing ✅
Active blocking reviews: None. Audacity88 approved (Member).

---

I reviewed the diff at 6b704db, the surrounding audit_markdown_link_target function in
crates/zeroclaw-runtime/src/skills/audit.rs (local source, lines 235–337), the test suite,
and the PR's security claims against the audit architecture. The fix is minimal, targeted, and
correct. Security analysis below.

---

🟢 The security boundary is correctly preserved

The core question for this PR is whether removing the .md-suffix check on http/https/mailto
links creates a bypass path for executable remote content. It does not, for the following
reasons verified against the local source:

1. No load-time fetch occurs. The audit function inspects link targets as strings. The
   loader never performs an HTTP request for markdown links in SKILL.md — they are read as
   prompt text. This is confirmed by the absence of any reqwest call in the skills/audit.rs
   code path and by the existing skills/ reqwest use (install-bundle download only, unrelated
   to audit).

2. Structural checks are unaffected. The scheme guard immediately below the removed branch
   still catches javascript:, file:, data:, and all other non-http(s)/mailto schemes with
   an "unsupported URL scheme" finding. The has_script_suffix check, looks_like_absolute_path
   check, and same-skill-root file existence check all operate only on the local-path branch
   (after the early return) and are untouched.

3. The .md suffix is not a reliable content-type signal. As the PR correctly notes,
   many documentation hosting platforms (Cloudinary, Sanity, GitHub Pages) serve HTML at .md
   routes. The check was not providing a meaningful security guarantee.

4. Downstream fetch remains tool-policy gated. If an agent later follows a link from
   skill prompt text, that action goes through the normal tool/network policy (domain guard,
   SSRF check, allowlist). The audit layer is not the right place to police post-load fetches.

---

🟢 Regression coverage covers the right risks

• audit_allows_remote_markdown_documentation_links: proves https://cloudinary.com/...transformation_reference.md,
  http://example.com/docs/schema.markdown, and mailto:support@example.com all audit clean.
  Uses tempfile::tempdir() (hermetic, no network). ✅
• audit_still_rejects_unsupported_url_schemes: proves javascript:alert(1) and
  file:///etc/passwd.md still produce exactly two "unsupported URL scheme in markdown link"
  findings. Covers the exact bypass scenario the PR risks. ✅

The two tests together pin the boundary: safe-for-prompt-text http/https/mailto links pass,
and scheme-dangerous links still fail. This is the right regression shape.

---

🟢 Validation evidence is comprehensive

cargo test --workspace --locked — 9456 passed, 0 failed, 67 test binaries, including both
new tests. cargo fmt, cargo clippy -D warnings both clean. Full workspace run is above and
beyond the minimum required.

---

🔵 Suggestion — The has_markdown_suffix helper is now unreferenced at the URL branch

has_markdown_suffix is still used in the local-path branch (line ~282: checks whether a
relative path link points to a .md file before attempting to canonicalize it). The PR body
correctly confirms "has_markdown_suffix is still referenced elsewhere (no dead code)" — the
local-path branch is the surviving caller. Just confirming this is consistent with the local
source.

---

Template completeness

Section	Status
Summary	✅
Validation Evidence	✅ (full workspace test run)
Security & Privacy	✅ (removal of check explained and justified)
Compatibility	✅
Rollback	✅
No AI trailers	✅
No bare string literals	✅