feat(skills): add skill-local safe_mode and resource classification #166

Merged
CoreRasurae merged 1 commit from feature/m1-skill-safe-mode-resource-classification into develop 2026-11-10 14:00:39 +00:00
Member

Summary

Adds the ADR-2034 D-8/D-9 foundation to the Skill Package Schema:

  • Optional top-level safe_mode boolean (default true); non-boolean values raise the registry ValidationError; absence stays canonical so existing Package IDs are unchanged; safe_mode: false is retained and changes the content-addressed ID.
  • Classifies every bundled resource by its first POSIX segment (references/, assets/, scripts/, non-standard, root file), case-sensitively.
  • Carries the resource class and effective safe_mode through Skill/SkillResource and the loaded skill catalogue.
  • Adds authoring documentation, an example skill package, and ASV benchmarks.
  • Introduces no script execution or privileged access.

Closes #161

Verification

  • nox -s unit_tests -- features/agent_skills.feature
  • nox -s integration_tests -- robot/agent_skills.robot
  • nox -s coverage_report — 97.2%
  • nox -s lint, nox -s format, nox -s typecheck, nox -s security_scan, nox -s dead_code
  • nox -s docs
  • nox -s benchmark -- --bench Skill
## Summary Adds the ADR-2034 D-8/D-9 foundation to the Skill Package Schema: - Optional top-level `safe_mode` boolean (default `true`); non-boolean values raise the registry `ValidationError`; absence stays canonical so existing Package IDs are unchanged; `safe_mode: false` is retained and changes the content-addressed ID. - Classifies every bundled resource by its first POSIX segment (`references/`, `assets/`, `scripts/`, non-standard, root file), case-sensitively. - Carries the resource class and effective `safe_mode` through `Skill`/`SkillResource` and the loaded skill catalogue. - Adds authoring documentation, an example skill package, and ASV benchmarks. - Introduces no script execution or privileged access. Closes #161 ## Verification - `nox -s unit_tests -- features/agent_skills.feature` - `nox -s integration_tests -- robot/agent_skills.robot` - `nox -s coverage_report` — 97.2% - `nox -s lint`, `nox -s format`, `nox -s typecheck`, `nox -s security_scan`, `nox -s dead_code` - `nox -s docs` - `nox -s benchmark -- --bench Skill`
Adds an optional `backends` list (>= 2 entries) and `strategy` field
(default and currently only supported value: `round_robin`) to `type: llm`
agent configuration, so a single logical agent can pool several backend
credential sets -- e.g. multiple OAuth subscriptions for the same provider
-- and forward each invocation to exactly one of them. This solves the
"single provider/model/credential-set per agent" limitation of the shared,
provider-keyed credentials dict, which cannot hold two distinct credential
sets for the same provider name.

Design is recorded in ADR-2038. Key decisions carried into this
implementation:
- `VirtualChatModel` is a genuine `BaseChatModel` subclass (same precedent
  as `ReasoningChatModel`, ADR-2036), overriding only
  `_generate`/`_agenerate`/`_stream`/`_astream`/`_llm_type` so it is
  Liskov-substitutable for any other chat model this project constructs.
- Each backend is built via the existing, unmodified per-provider
  construction/credential-validation logic
  (`llm_client._build_from_credentials`) -- no parallel validation path.
- Backend selection happens exactly once per external invocation; retries
  (ADR-2032) and the first-chunk-only streaming retry are performed
  internally against that same selected backend, never re-selecting
  mid-retry or mid-stream -- otherwise a retry would silently bounce
  between backends, which the issue explicitly calls out as must-not-happen.
  `LLMAgent._retry_ainvoke`/`_run_pruning_pass`/`stream_message` needed no
  changes; their own outer retry wrapping becomes a harmless no-op for a
  virtual provider since `VirtualChatModel` already exhausts retries
  internally before raising.
- `reasoning_feedback` is read once at the virtual-provider level and
  applied identically to every backend, so a reasoning-aware backend
  switch still round-trips `reasoning_content` correctly.

Also fixes a pre-existing Behave test-isolation bug surfaced while adding
these scenarios: `reasoning_provider_routing_steps.py`'s
`_reasoning_unavailable` context flag wasn't reset in `before_scenario`, so
it silently leaked into any later scenario reusing the shared "the chat
model is constructed" step, breaking unrelated native-provider construction.

Module: src/cleveractors/agents/llm_virtual_provider.py (new),
src/cleveractors/agents/llm_client.py, src/cleveractors/agents/llm.py,
src/cleveractors/validation/_agents.py, src/cleveractors/validation/_vocabulary.py.
BDD: features/virtual_provider.feature. Robot: robot/virtual_provider_routing.robot.
Benchmark: benchmarks/virtual_provider_benchmark.py.
Example: examples/packages/virtual-provider-pooled-agent.yaml.

ISSUES CLOSED: #155
docs: Document virtual providers for load balancing
Some checks failed
CI / lint (pull_request) Successful in 35s
CI / quality (pull_request) Successful in 1m10s
CI / typecheck (pull_request) Successful in 2m10s
CI / security (pull_request) Successful in 2m23s
CI / build (pull_request) Successful in 1m49s
CI / integration_tests (pull_request) Successful in 3m45s
CI / unit_tests (pull_request) Successful in 4m59s
CI / benchmark (pull_request) Has been cancelled
CI / coverage (pull_request) Successful in 4m36s
CI / status-check (pull_request) Successful in 1s
CI / lint (push) Successful in 1m11s
CI / build (push) Successful in 1m11s
CI / quality (push) Successful in 1m44s
CI / security (push) Successful in 2m4s
CI / integration_tests (push) Successful in 4m42s
CI / unit_tests (push) Successful in 6m43s
CI / typecheck (push) Failing after 11m15s
CI / benchmark (push) Failing after 23m33s
CI / coverage (push) Has been skipped
CI / status-check (push) Failing after 4s
9631ee670a
Refs: #155
feat(executor): let provably zero-cost invocations pass the budget pre-flight
Some checks failed
CI / lint (pull_request) Successful in 1m5s
CI / typecheck (pull_request) Successful in 1m32s
CI / quality (pull_request) Successful in 1m32s
CI / build (pull_request) Successful in 2m19s
CI / security (pull_request) Successful in 3m3s
CI / integration_tests (pull_request) Successful in 3m39s
CI / benchmark (pull_request) Failing after 19m14s
CI / unit_tests (pull_request) Failing after 19m28s
CI / lint (push) Successful in 32s
CI / typecheck (push) Successful in 1m6s
CI / security (push) Successful in 55s
CI / quality (push) Successful in 1m5s
CI / build (push) Successful in 36s
CI / integration_tests (push) Failing after 3m42s
CI / unit_tests (push) Successful in 5m35s
CI / benchmark (push) Failing after 13m33s
CI / coverage (push) Failing after 13m19s
CI / coverage (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
CI / status-check (push) Has been cancelled
4a9818cbdf
The max_cost_usd pre-flight gate raises budget_exhausted when
accumulated >= limit before an invocation (issue #76, PR #80). A fresh
execution starts at accumulated = 0, so a 0.0 limit (the embedding-
platform case max_cost_usd = min(platform_cap, wallet.available_balance)
on an empty wallet) tripped the gate before the first node for every
run, blocking runs whose every LLM invocation is provably free — a real,
growing class of catalogued 0/0-rate models (big-pickle, glm-5-free).
Consumers had to ship a platform-side static zero-cost estimator purely
to work around it (cleveragents/cleveragents-webapp#531).

The gate now resolves the upcoming invocation's (provider, model) rates
through the exact ladder the gate charges by — lifted from _compute_cost
into core.exceptions._resolve_pricing_rates with a conservative
_is_zero_rated_invocation predicate — and lets the invocation pass the
tripped gate only when both resolved rates are exactly 0.0. Anything
unresolvable still blocks: missing/incomplete/invalid entries, half-zero
entries, non-AGENT nodes, non-LLM agents, unresolved agent names, empty
pricing tables (issue #150 AC5, the same conservative default the
platform-side estimator applies). PR #80 semantics are preserved: post-
node/post-invocation checks stay >, so an exactly-at-limit node still
completes and only the next payable node/invocation is refused.

Applied at all three pre-flight enforcement points:
- PureLangGraph._check_budget_pre_flight (per node, execute and stream
  share it via _node_budget_context), with the conservative taxonomy
  helper _upcoming_node_invocation_is_zero_rated;
- LLMAgent._check_budget_before_invoke (tool-loop pre-invoke, correct
  for all five call sites since each precedes self.chat_model);
  purely-suppressant _should_skip_pruning is deliberately unchanged
  (ADR-2038 D-3: a free invocation does not resurrect pruning);
- new shared runtime_dispatch._check_single_llm_budget_pre_flight for
  both single-type:llm dispatch functions (AC4). In _execute_llm_stream
  it raises inside the early-validation wrapper so the billing-integrity
  executor.last_result placeholder is still populated.

Owner-approved full alignment also closes a discovered enforcement gap:
the non-stream _execute_llm path had no max_cost_usd enforcement at all
(neither pre-flight nor post-invocation). It now mirrors the stream and
graph shapes with the post-invocation > cost check, the bool/non-numeric
limit ladder, and the ADR-2029 non-empty-pricing-table entry validation,
so the same config no longer succeeds or fails depending on which
executor shape it lands in.

Docs: new ADR-2038 (docs/adr/ADR-2038-budget-gate-zero-cost-carveout.md,
continuing the ADR-2029/PR-80 budget lineage) and a CHANGELOG.md
[Unreleased] entry; no pyproject version bump since v2.1.0 is still
unreleased (no tags; every prior milestone feature also landed under
[Unreleased]) and the release process is tag-driven.

Tests: 21 new Behave scenarios (features/execution_limits.feature graph
path incl. the payable-then-free-then-payable chain, features/
llm_agent_tool_loop.feature tool-loop path incl. pruning-skip and
no-pricing-table locks, new features/budget_gate_single_llm.feature
single-LLM execute/stream with invocation-counting mock models) plus 7
new Robot integration tests (robot/budget_gate_zero_cost.robot) driving
real create_executor actors end to end. The two single-LLM stream
scenarios whose enforcement point moved from post-stream to pre-flight
were updated to cover both gates separately. Gates: nox format, lint,
typecheck, security_scan, dead_code, unit_tests (3136 scenarios),
integration_tests (373 tests), coverage_report all pass; coverage
improved from 96.90% (origin/master, measured) to 97.11%, above the
97% standard, with every new line covered.

ISSUES CLOSED: #150
feat(agents): add random backend-selection strategy for virtual provider LLM agents
Some checks failed
CI / lint (pull_request) Successful in 1m13s
CI / security (pull_request) Successful in 2m12s
CI / build (pull_request) Successful in 2m9s
CI / typecheck (pull_request) Successful in 2m50s
CI / quality (pull_request) Successful in 2m56s
CI / integration_tests (pull_request) Successful in 4m1s
CI / unit_tests (pull_request) Successful in 6m49s
CI / coverage (pull_request) Successful in 6m11s
CI / status-check (pull_request) Successful in 2s
CI / benchmark (pull_request) Failing after 17m14s
CI / lint (push) Successful in 40s
CI / typecheck (push) Successful in 2m2s
CI / quality (push) Successful in 2m8s
CI / security (push) Successful in 2m37s
CI / build (push) Successful in 1m26s
CI / integration_tests (push) Successful in 4m12s
CI / unit_tests (push) Successful in 6m26s
CI / coverage (push) Successful in 5m33s
CI / status-check (push) Successful in 6s
CI / benchmark (push) Failing after 24m16s
4b98ce0067
Extends the backends/strategy virtual-provider routing (#155, ADR-2038)
with the second required selection strategy: strategy: random selects a
backend uniformly at random, via Python's random module, with identical
selection-and-forward semantics to round_robin -- only how the backend
index is chosen differs.

RandomStrategy needs no concurrency guard (unlike RoundRobinStrategy's
position counter): random.randrange has no shared mutable state to
protect. The non-cryptographic random source is deliberate -- this is a
capacity-distribution concern, not a security boundary -- documented with
a `# nosec B311` justification comment for the expected low-severity
bandit finding.

Two existing scenarios used "random" as an example of an *unsupported*
strategy value; both now use "weighted" instead, since random is no
longer unsupported.

The num_backends <= 0 guard is hoisted into a shared
SelectionStrategy._validate_num_backends helper instead of being
duplicated in both RoundRobinStrategy and RandomStrategy. Behave and
Robot coverage for the random path now assert the selected backend's
response is forwarded unmodified, not just that it was called. The ASV
benchmark helper's strategy parameter is typed as the SelectionStrategy
ABC instead of the concrete union, and the Robot suite's docstrings now
mention RandomStrategy alongside RoundRobinStrategy.

ISSUES CLOSED: #156
feat(skills): add skill-local safe_mode and resource classification
Some checks failed
CI / lint (pull_request) Successful in 1m12s
CI / typecheck (pull_request) Successful in 1m15s
CI / security (pull_request) Successful in 1m35s
CI / quality (pull_request) Successful in 2m38s
CI / build (pull_request) Successful in 2m8s
CI / unit_tests (pull_request) Successful in 6m20s
CI / coverage (pull_request) Successful in 5m50s
CI / benchmark (pull_request) Failing after 28m52s
CI / integration_tests (pull_request) Failing after 2m37s
CI / status-check (pull_request) Failing after 2s
073ea04d7a
Add the ADR-2034 D-8/D-9 foundation to the Skill Package Schema. A skill
may declare an optional boolean safe_mode (default true); non-boolean
values fail with the existing registry ValidationError, absence remains
canonical so existing Package IDs are unchanged, and safe_mode: false is
retained and participates in the content-addressed identity.

Classify every bundled resource path by its first POSIX segment into
references/, assets/, scripts/, non-standard, or root-file classes and
carry the class plus effective safe_mode through Skill, SkillResource,
and the loaded skill catalogue.

Document the authoring rules in a new guide, add an example skill package,
and add ASV benchmarks for classification and schema validation. No
script execution or privileged access is introduced by this change.

ISSUES CLOSED: #161
CoreRasurae added this to the v2.1.0 milestone 2026-11-09 16:51:11 +00:00
Member

Since the PR likely has a wrong target branch (feature branch cut off from develop but PR merges into master), so the review agent gives a corrupted report. I'll pick the related stuff here.

Changes in commit 073ea04d7a are mostly solid, but the ADRs are kinda broken. The code and comments referenced ADR-2034 D-8/D-9, but we have two ADR-2034 and the related one has only D1 to D-7, there is no D8 and D9. Due to this, I think my review agent failed to review the code because there is no spec to compare against.

The minor issue and nits:

Minor Issues

  1. Backslash paths silently classify as root files — src/cleveractors/agents/skill_schema.py lines 79–84 (classify) and 353–366 (_validate_resources). Path validation (pre-existing, unchanged here) rejects leading / and ../ segments but not \; a path like scripts\run.py contains no / and classifies as ROOT_FILE, which would let it bypass the future scripts/ policy gate. No enforcement exists yet in this PR, so impact is deferred — Recommendation: reject \ in resource-path validation (or classify such paths NON_STANDARD) now, or explicitly note it in the upcoming enforcement issue.

Nits

  1. src/cleveractors/agents/skill_schema.py lines 77–78 — the classify() non-string/empty-path ValueError guard is the file's only uncovered line (99.6% file coverage). Defensive-only (the validator pre-validates paths); acceptable to leave.
Since the PR likely has a wrong target branch (feature branch cut off from develop but PR merges into master), so the review agent gives a corrupted report. I'll pick the related stuff here. Changes in commit 073ea04d7a5b1991aa17e813babf24030a52e026 are mostly solid, but the ADRs are kinda broken. The code and comments referenced `ADR-2034 D-8/D-9`, but we have two ADR-2034 and the related one has only D1 to D-7, there is no D8 and D9. Due to this, I think my review agent failed to review the code because there is no spec to compare against. The minor issue and nits: #### Minor Issues 1. **Backslash paths silently classify as root files —** `src/cleveractors/agents/skill_schema.py` lines 79–84 (`classify`) and 353–366 (`_validate_resources`). Path validation (pre-existing, unchanged here) rejects leading `/` and `../` segments but not `\`; a path like `scripts\run.py` contains no `/` and classifies as `ROOT_FILE`, which would let it bypass the future `scripts/` policy gate. No enforcement exists yet in this PR, so impact is deferred — **Recommendation:** reject `\` in resource-path validation (or classify such paths `NON_STANDARD`) now, or explicitly note it in the upcoming enforcement issue. #### Nits 1. `src/cleveractors/agents/skill_schema.py` lines 77–78 — the `classify()` non-string/empty-path `ValueError` guard is the file's only uncovered line (99.6% file coverage). Defensive-only (the validator pre-validates paths); acceptable to leave.
CoreRasurae changed target branch from master to develop 2026-11-10 08:34:59 +00:00
hurui200320 requested changes 2026-11-10 08:44:44 +00:00
Dismissed
hurui200320 left a comment

PR Review: !166 (Ticket #161)

Verdict: Request Changes

The implementation code is solid — validation order, ID canonicalization, catalogue plumbing, and tests are all correct and well-structured. However, the PR ships references to design decisions (ADR-2034 D-8/D-9/D-10) that are not recorded anywhere, and leaves the normative Skill Package Schema (§16.1/§16.2) unupdated while the new guide explicitly defers normativity to it. Both were flagged in the earlier comment and remain unaddressed; the author has not yet responded, so they stay open.

Scope notes: the wrong-target-branch problem mentioned earlier is resolved (base is now develop, matching the branch point) and is dropped. Red-tape items (commit message, labels, atomicity, branch name) all check out and are excluded per instructions. No author responses exist on the PR or #161, so no item has been formally deferred by the author.

Critical Issues

None

Major Issues

  1. ADR-2034 D-8/D-9/D-10 are cited throughout but do not exist in the ADR.
    docs/adr/ADR-2034-skill-package-support.md records only D-1 through D-7. The following cite the nonexistent decisions:

    • src/cleveractors/agents/skill_schema.py:41, skill_schema.py:51 (docstrings: "ADR-2034 D-8")
    • docs/guides/skill-packages.md:6 and docs/guides/skill-packages.md:122 (D-8, D-9, D-10)
    • benchmarks/skill_schema_benchmark.py:4, benchmarks/skill_schema_benchmark.py:21
    • robot/agent_skills.robot (test documentation), examples/README.md

    The taxonomy and safe-mode decisions are the entire normative basis of this PR, yet no ADR records them. (The pre-existing ADR-2034 numbering collision with ADR-2034-content-not-contains-all-subsystems.md makes the dangling references more confusing.)
    Recommendation: amend ADR-2034 with the D-8 (resource taxonomy) and D-9 (skill-local safe_mode) decisions — or add a follow-up ADR — before merge, so the code and guide cite a decision that actually exists.

  2. The normative schema was not updated: §16.1 field table and §16.2 identity enumeration lack safe_mode.
    docs/guides/skill-packages.md:8-9 states "The normative field table remains Package Registry Standard §16.1", but docs/actor-registry-standard.md §16.1's field table has no safe_mode row, and §16.2's identity discussion enumerates the hashed fields (skill/name/description/.../resources) without safe_mode — even though this PR makes safe_mode: false part of the content-addressed identity.
    Recommendation: add the safe_mode row (optional boolean, default true, non-boolean rejected) to §16.1 and include it in §16.2's identity enumeration.

Minor Issues

  1. Backslash paths silently classify as ROOT_FILE.
    src/cleveractors/agents/skill_schema.py:353-366 (_validate_resources) and skill_schema.py:79-84 (classify): path validation rejects leading / and ./.. segments but not \\; a path like scripts\\run.py contains no / and classifies as ROOT_FILE, which would let it bypass the future scripts/ policy gate. No enforcement exists yet in this PR, so impact is deferred — as noted in the earlier comment — but this PR is the foundation that classification is built on.
    Recommendation: reject \\ in resource-path validation (or classify such paths NON_STANDARD) now, or explicitly record it in the upcoming enforcement issue.

Nits

  1. src/cleveractors/agents/skill_schema.py:77-78 — the classify() non-string/empty-path ValueError guard is the file's only uncovered code (defensive-only; the validator pre-validates paths). A one-line scenario asserting the ValueError would bring the file to 100%.

Summary

Well-implemented foundation: the classifier is a clean table-driven Strategy, safe_mode validation/canonicalization is correct (absence ≡ true for ID purposes, false retained and identity-changing, validated before ID computation), the catalogue picks up the new fields automatically via to_context_dict() with no changes needed in SkillLoader, no other SkillResource construction sites break, and the skill tool tolerates the additive class key. Behave and Robot coverage is meaningful, including case-sensitivity and package-ID equivalence scenarios. The blockers are purely documentary but matter in a docs-first project: record D-8/D-9 in the ADR and update §16.1/§16.2, then this is ready to merge.

## PR Review: !166 (Ticket #161) ### Verdict: Request Changes The implementation code is solid — validation order, ID canonicalization, catalogue plumbing, and tests are all correct and well-structured. However, the PR ships references to design decisions (**ADR-2034 D-8/D-9/D-10**) that are not recorded anywhere, and leaves the normative Skill Package Schema (§16.1/§16.2) unupdated while the new guide explicitly defers normativity to it. Both were flagged in the earlier comment and remain unaddressed; the author has not yet responded, so they stay open. *Scope notes:* the wrong-target-branch problem mentioned earlier is resolved (base is now `develop`, matching the branch point) and is dropped. Red-tape items (commit message, labels, atomicity, branch name) all check out and are excluded per instructions. No author responses exist on the PR or #161, so no item has been formally deferred by the author. ### Critical Issues None ### Major Issues 1. **ADR-2034 D-8/D-9/D-10 are cited throughout but do not exist in the ADR.** `docs/adr/ADR-2034-skill-package-support.md` records only D-1 through D-7. The following cite the nonexistent decisions: - `src/cleveractors/agents/skill_schema.py:41`, `skill_schema.py:51` (docstrings: "ADR-2034 D-8") - `docs/guides/skill-packages.md:6` and `docs/guides/skill-packages.md:122` (D-8, D-9, D-10) - `benchmarks/skill_schema_benchmark.py:4`, `benchmarks/skill_schema_benchmark.py:21` - `robot/agent_skills.robot` (test documentation), `examples/README.md` The taxonomy and safe-mode decisions are the entire normative basis of this PR, yet no ADR records them. (The pre-existing ADR-2034 numbering collision with `ADR-2034-content-not-contains-all-subsystems.md` makes the dangling references more confusing.) **Recommendation:** amend ADR-2034 with the D-8 (resource taxonomy) and D-9 (skill-local `safe_mode`) decisions — or add a follow-up ADR — before merge, so the code and guide cite a decision that actually exists. 2. **The normative schema was not updated: §16.1 field table and §16.2 identity enumeration lack `safe_mode`.** `docs/guides/skill-packages.md:8-9` states "The normative field table remains Package Registry Standard §16.1", but `docs/actor-registry-standard.md` §16.1's field table has no `safe_mode` row, and §16.2's identity discussion enumerates the hashed fields (`skill`/`name`/`description`/.../`resources`) without `safe_mode` — even though this PR makes `safe_mode: false` part of the content-addressed identity. **Recommendation:** add the `safe_mode` row (optional boolean, default `true`, non-boolean rejected) to §16.1 and include it in §16.2's identity enumeration. ### Minor Issues 1. **Backslash paths silently classify as `ROOT_FILE`.** `src/cleveractors/agents/skill_schema.py:353-366` (`_validate_resources`) and `skill_schema.py:79-84` (`classify`): path validation rejects leading `/` and `.`/`..` segments but not `\\`; a path like `scripts\\run.py` contains no `/` and classifies as `ROOT_FILE`, which would let it bypass the future `scripts/` policy gate. No enforcement exists yet in this PR, so impact is deferred — as noted in the earlier comment — but this PR is the foundation that classification is built on. **Recommendation:** reject `\\` in resource-path validation (or classify such paths `NON_STANDARD`) now, or explicitly record it in the upcoming enforcement issue. ### Nits 1. `src/cleveractors/agents/skill_schema.py:77-78` — the `classify()` non-string/empty-path `ValueError` guard is the file's only uncovered code (defensive-only; the validator pre-validates paths). A one-line scenario asserting the `ValueError` would bring the file to 100%. ### Summary Well-implemented foundation: the classifier is a clean table-driven Strategy, `safe_mode` validation/canonicalization is correct (absence ≡ `true` for ID purposes, `false` retained and identity-changing, validated before ID computation), the catalogue picks up the new fields automatically via `to_context_dict()` with no changes needed in `SkillLoader`, no other `SkillResource` construction sites break, and the skill tool tolerates the additive `class` key. Behave and Robot coverage is meaningful, including case-sensitivity and package-ID equivalence scenarios. The blockers are purely documentary but matter in a docs-first project: record D-8/D-9 in the ADR and update §16.1/§16.2, then this is ready to merge.
@ -0,0 +3,4 @@
A `skill` package is a single content-addressed YAML mapping that carries a
skill's metadata, instructions, and bundled files. This guide covers the
skill-local safety switch and the resource-directory taxonomy added by
[ADR-2034](../adr/ADR-2034-skill-package-support.md) (D-8 and D-9).
Member

Major: ADR-2034 (skill-package-support) records only D-1..D-7 — D-8/D-9/D-10 cited here (and in skill_schema.py docstrings, benchmarks, robot docs, examples/README) do not exist. Please amend the ADR (or add a follow-up ADR) recording these decisions before merge.

Major: ADR-2034 (skill-package-support) records only D-1..D-7 — D-8/D-9/D-10 cited here (and in `skill_schema.py` docstrings, benchmarks, robot docs, examples/README) do not exist. Please amend the ADR (or add a follow-up ADR) recording these decisions before merge.
@ -0,0 +6,4 @@
[ADR-2034](../adr/ADR-2034-skill-package-support.md) (D-8 and D-9).
The normative field table remains
[Package Registry Standard §16.1](../actor-registry-standard.md#161-skill-package-schema).
Member

Major: this guide defers normativity to §16.1 of docs/actor-registry-standard.md, but that field table was not updated in this PR — it has no safe_mode row, and §16.2's identity enumeration does not include it even though safe_mode: false now changes the Package ID. Please update both sections.

Major: this guide defers normativity to §16.1 of `docs/actor-registry-standard.md`, but that field table was not updated in this PR — it has no `safe_mode` row, and §16.2's identity enumeration does not include it even though `safe_mode: false` now changes the Package ID. Please update both sections.
@ -39,0 +74,4 @@
from the first segment, or ``NON_STANDARD`` when it is not one of
the three standard directories.
"""
if not isinstance(path, str) or not path:
Member

Nit: this defensive ValueError guard is the only uncovered code in the file; a one-line scenario asserting the error would bring the file to 100%.

Nit: this defensive ValueError guard is the only uncovered code in the file; a one-line scenario asserting the error would bring the file to 100%.
Member

Minor (deferred impact, flagged earlier): path validation rejects leading / and ./.. segments but not \\; scripts\\run.py has no / and classifies as ROOT_FILE, which would bypass the future scripts/ policy gate. Recommend rejecting \\ here (or classifying such paths NON_STANDARD), or explicitly noting it in the upcoming enforcement issue.

Minor (deferred impact, flagged earlier): path validation rejects leading `/` and `.`/`..` segments but not `\\`; `scripts\\run.py` has no `/` and classifies as ROOT_FILE, which would bypass the future `scripts/` policy gate. Recommend rejecting `\\` here (or classifying such paths NON_STANDARD), or explicitly noting it in the upcoming enforcement issue.
hurui200320 requested changes 2026-11-10 08:44:52 +00:00
Dismissed
hurui200320 left a comment

Review complete — see the report above. Two major documentation/spec-integrity issues (missing ADR-2034 D-8/D-9 records; §16.1/§16.2 not updated) block the merge; code itself looks correct.

Review complete — see the report above. Two major documentation/spec-integrity issues (missing ADR-2034 D-8/D-9 records; §16.1/§16.2 not updated) block the merge; code itself looks correct.
@ -0,0 +3,4 @@
A `skill` package is a single content-addressed YAML mapping that carries a
skill's metadata, instructions, and bundled files. This guide covers the
skill-local safety switch and the resource-directory taxonomy added by
[ADR-2034](../adr/ADR-2034-skill-package-support.md) (D-8 and D-9).
Member

Major: ADR-2034 (skill-package-support) records only D-1..D-7 — D-8/D-9/D-10 cited here (and in skill_schema.py docstrings, benchmarks, robot docs, examples/README) do not exist. Please amend the ADR (or add a follow-up ADR) recording these decisions before merge.

Major: ADR-2034 (skill-package-support) records only D-1..D-7 — D-8/D-9/D-10 cited here (and in `skill_schema.py` docstrings, benchmarks, robot docs, examples/README) do not exist. Please amend the ADR (or add a follow-up ADR) recording these decisions before merge.
@ -0,0 +6,4 @@
[ADR-2034](../adr/ADR-2034-skill-package-support.md) (D-8 and D-9).
The normative field table remains
[Package Registry Standard §16.1](../actor-registry-standard.md#161-skill-package-schema).
Member

Major: this guide defers normativity to §16.1 of docs/actor-registry-standard.md, but that field table was not updated in this PR — it has no safe_mode row, and §16.2's identity enumeration does not include it even though safe_mode: false now changes the Package ID. Please update both sections.

Major: this guide defers normativity to §16.1 of `docs/actor-registry-standard.md`, but that field table was not updated in this PR — it has no `safe_mode` row, and §16.2's identity enumeration does not include it even though `safe_mode: false` now changes the Package ID. Please update both sections.
@ -39,0 +74,4 @@
from the first segment, or ``NON_STANDARD`` when it is not one of
the three standard directories.
"""
if not isinstance(path, str) or not path:
Member

Nit: this defensive ValueError guard is the only uncovered code in the file; a one-line scenario asserting the error would bring the file to 100%.

Nit: this defensive ValueError guard is the only uncovered code in the file; a one-line scenario asserting the error would bring the file to 100%.
Member

Minor (deferred impact, flagged earlier): path validation rejects leading / and ./.. segments but not \\; scripts\\run.py has no / and classifies as ROOT_FILE, which would bypass the future scripts/ policy gate. Recommend rejecting \\ here (or classifying such paths NON_STANDARD), or explicitly noting it in the upcoming enforcement issue.

Minor (deferred impact, flagged earlier): path validation rejects leading `/` and `.`/`..` segments but not `\\`; `scripts\\run.py` has no `/` and classifies as ROOT_FILE, which would bypass the future `scripts/` policy gate. Recommend rejecting `\\` here (or classifying such paths NON_STANDARD), or explicitly noting it in the upcoming enforcement issue.
CoreRasurae force-pushed feature/m1-skill-safe-mode-resource-classification from 073ea04d7a
Some checks failed
CI / lint (pull_request) Successful in 1m12s
CI / typecheck (pull_request) Successful in 1m15s
CI / security (pull_request) Successful in 1m35s
CI / quality (pull_request) Successful in 2m38s
CI / build (pull_request) Successful in 2m8s
CI / unit_tests (pull_request) Successful in 6m20s
CI / coverage (pull_request) Successful in 5m50s
CI / benchmark (pull_request) Failing after 28m52s
CI / integration_tests (pull_request) Failing after 2m37s
CI / status-check (pull_request) Failing after 2s
to 98fbb98863
Some checks failed
CI / lint (pull_request) Successful in 1m27s
CI / typecheck (pull_request) Successful in 1m54s
CI / unit_tests (pull_request) Has started running
CI / benchmark (pull_request) Has started running
CI / build (pull_request) Successful in 1m54s
CI / security (pull_request) Successful in 2m21s
CI / quality (pull_request) Successful in 2m27s
CI / integration_tests (pull_request) Successful in 4m14s
CI / coverage (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
2026-11-10 11:27:01 +00:00
Compare
CoreRasurae force-pushed feature/m1-skill-safe-mode-resource-classification from 98fbb98863
Some checks failed
CI / lint (pull_request) Successful in 1m27s
CI / typecheck (pull_request) Successful in 1m54s
CI / unit_tests (pull_request) Has started running
CI / benchmark (pull_request) Has started running
CI / build (pull_request) Successful in 1m54s
CI / security (pull_request) Successful in 2m21s
CI / quality (pull_request) Successful in 2m27s
CI / integration_tests (pull_request) Successful in 4m14s
CI / coverage (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
to 939e93d391
Some checks failed
CI / typecheck (pull_request) Has started running
CI / security (pull_request) Has started running
CI / quality (pull_request) Has started running
CI / unit_tests (pull_request) Has started running
CI / integration_tests (pull_request) Has started running
CI / coverage (pull_request) Has been cancelled
CI / build (pull_request) Has been cancelled
CI / benchmark (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
CI / lint (pull_request) Has been cancelled
2026-11-10 11:35:36 +00:00
Compare
CoreRasurae force-pushed feature/m1-skill-safe-mode-resource-classification from 939e93d391
Some checks failed
CI / typecheck (pull_request) Has started running
CI / security (pull_request) Has started running
CI / quality (pull_request) Has started running
CI / unit_tests (pull_request) Has started running
CI / integration_tests (pull_request) Has started running
CI / coverage (pull_request) Has been cancelled
CI / build (pull_request) Has been cancelled
CI / benchmark (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
CI / lint (pull_request) Has been cancelled
to ae7b44734a
Some checks failed
CI / lint (pull_request) Successful in 1m4s
CI / quality (pull_request) Successful in 1m25s
CI / typecheck (pull_request) Successful in 1m33s
CI / security (pull_request) Successful in 1m56s
CI / build (pull_request) Successful in 2m12s
CI / integration_tests (pull_request) Successful in 3m59s
CI / unit_tests (pull_request) Successful in 5m26s
CI / coverage (pull_request) Successful in 5m47s
CI / status-check (pull_request) Successful in 1s
CI / benchmark (pull_request) Failing after 18m43s
2026-11-10 11:36:03 +00:00
Compare
hurui200320 left a comment

PR Review: !166 (Ticket #161)

Verdict: Approve

All four findings from the earlier (now stale) REQUEST_CHANGES review were addressed in the reworked single commit ae7b447: the ADR-2034 amendment now records D-8/D-9/D-10 with rationale and revision history; Package Registry Standard §16.1/§16.2 are updated with the 1.2.0 version bump; backslash paths are now rejected by validation; and the classifier's defensive guard is covered by a scenario. The implementation matches the ticket and the amended spec, the Behave/Robot tests are meaningful and cover every acceptance criterion, and all blocking CI gates pass (the benchmark job failure is pre-existing on develop itself and non-gating by design — continue-on-error: true, excluded from status-check).

Note on prior discussion: the PR author left no text comments; the response came as the reworked commit, which resolved every prior finding (ADR records, §16 updates, backslash rejection, guard coverage) and the PR is now correctly based on/merged into develop (branch cut from 4b98ce0 = develop head).

Critical Issues

None.

Major Issues

None.

Minor Issues

None.

Nits

  1. Stale follow-up items in the amended ADR — docs/adr/ADR-2034-skill-package-support.md, "Follow-up Required" section: the "Specification updates (revision)" bullet still asks to add safe_mode and the D-8 taxonomy to §16.1/§16.3 "(intended Version 1.2.0)", and the "Revision implementation (D-8–D-10)" bullet lumps in the schema-side work (safe_mode through Skill/SkillValidator/SkillLoader, path classification) that this PR itself completes. Only the D-10 enforcement half (llm_tools execute/args/timeout, the double-safety gate plus temporary-tree execution in ToolAgent._skill_tool) actually remains open. Recommend splitting these bullets into "done by #161" vs "remaining (D-10 enforcement)" so the follow-up list stays accurate.
  2. safe_mode naming ambiguity in the new guide — docs/guides/skill-packages.md ("Related documentation"): safe_mode now denotes two distinct switches — the agent-level safe_mode (§4.5, docs/tools/safe-mode.md) and the new skill-level field (§16.1). The guide states the double-gate relationship in prose; a one-line disambiguation or a cross-link to the agent-level safe-mode guide would prevent confusion for readers arriving from either document.

Scope confirmations (per prior discussion — not re-reported, excluded from this review)

  • D-10 enforcement (skill-tool execute action, double-safety gate in ToolAgent._skill_tool, temporary-tree materialization) is explicitly deferred by the ADR's "Follow-up Required" and the ticket's "no script execution" scope — correctly out of scope here.
  • Packer support (pack_skill_directory preserving directories and emitting safe_mode only when false) — ADR follow-up, does not exist yet in the codebase; out of scope.
  • PR target branch — the earlier "wrong target branch" concern is resolved; the PR now targets develop and is based on its head.
  • CI benchmark job failure — pre-existing: the same job fails on the develop head commit (4b98ce0) for both push and pull_request events, before this PR's benchmark file existed on that branch; the job is continue-on-error: true and excluded from the status-check gate, which passed. Not attributable to this PR.

Summary

A clean, well-scoped foundation PR for ADR-2034 D-8/D-9. skill_schema.py adds the SkillResourceClass enum and a table-driven, case-sensitive SkillResourceClassifier; safe_mode validation raises the registry ValidationError for non-boolean values (including null and 1); absence stays canonical for the Package ID while an explicit true is normalized away and false is retained, exactly per D-9/D-3. The pre-existing backslash gap is closed (validator, error message, tests, and docs all agree). SkillLoader needs no changes — the catalogue serializes to_context_dict(), which now carries safe_mode and per-resource class; the only consumer (ToolAgent._skill_tool) reads content/encoding keys, so the additions are inert. Tests are genuinely assertive: ID equality/difference is checked against an independently computed canonical ID, all five classes plus the Scripts/ case-sensitivity rule are covered, and the loaded-catalogue exposure is verified in both Behave and Robot. Documentation is thorough and internally consistent (ADR D-8/D-9/D-10 + alternatives A-7..A-13 + revision history; standard 1.2.0 field table, classification paragraph, §16.2 identity paragraph, §16.3 example; new authoring guide wired into mkdocs nav; example package; CHANGELOG entry). Coverage gating and all required CI jobs pass. Only two documentation nits remain, neither blocking.

## PR Review: !166 (Ticket #161) ### Verdict: Approve All four findings from the earlier (now stale) REQUEST_CHANGES review were addressed in the reworked single commit `ae7b447`: the ADR-2034 amendment now records D-8/D-9/D-10 with rationale and revision history; Package Registry Standard §16.1/§16.2 are updated with the 1.2.0 version bump; backslash paths are now rejected by validation; and the classifier's defensive guard is covered by a scenario. The implementation matches the ticket and the amended spec, the Behave/Robot tests are meaningful and cover every acceptance criterion, and all blocking CI gates pass (the `benchmark` job failure is pre-existing on `develop` itself and non-gating by design — `continue-on-error: true`, excluded from `status-check`). Note on prior discussion: the PR author left no text comments; the response came as the reworked commit, which resolved every prior finding (ADR records, §16 updates, backslash rejection, guard coverage) and the PR is now correctly based on/merged into `develop` (branch cut from `4b98ce0` = develop head). ### Critical Issues None. ### Major Issues None. ### Minor Issues None. ### Nits 1. **Stale follow-up items in the amended ADR** — `docs/adr/ADR-2034-skill-package-support.md`, "Follow-up Required" section: the "Specification updates (revision)" bullet still asks to add `safe_mode` and the D-8 taxonomy to §16.1/§16.3 "(intended Version 1.2.0)", and the "Revision implementation (D-8–D-10)" bullet lumps in the schema-side work (`safe_mode` through `Skill`/`SkillValidator`/`SkillLoader`, path classification) that this PR itself completes. Only the D-10 enforcement half (llm_tools `execute`/`args`/`timeout`, the double-safety gate plus temporary-tree execution in `ToolAgent._skill_tool`) actually remains open. Recommend splitting these bullets into "done by #161" vs "remaining (D-10 enforcement)" so the follow-up list stays accurate. 2. **`safe_mode` naming ambiguity in the new guide** — `docs/guides/skill-packages.md` ("Related documentation"): `safe_mode` now denotes two distinct switches — the agent-level `safe_mode` (§4.5, `docs/tools/safe-mode.md`) and the new skill-level field (§16.1). The guide states the double-gate relationship in prose; a one-line disambiguation or a cross-link to the agent-level safe-mode guide would prevent confusion for readers arriving from either document. ### Scope confirmations (per prior discussion — not re-reported, excluded from this review) - **D-10 enforcement** (skill-tool `execute` action, double-safety gate in `ToolAgent._skill_tool`, temporary-tree materialization) is explicitly deferred by the ADR's "Follow-up Required" and the ticket's "no script execution" scope — correctly out of scope here. - **Packer support** (`pack_skill_directory` preserving directories and emitting `safe_mode` only when false) — ADR follow-up, does not exist yet in the codebase; out of scope. - **PR target branch** — the earlier "wrong target branch" concern is resolved; the PR now targets `develop` and is based on its head. - **CI `benchmark` job failure** — pre-existing: the same job fails on the `develop` head commit (4b98ce0) for both push and pull_request events, before this PR's benchmark file existed on that branch; the job is `continue-on-error: true` and excluded from the `status-check` gate, which passed. Not attributable to this PR. ### Summary A clean, well-scoped foundation PR for ADR-2034 D-8/D-9. `skill_schema.py` adds the `SkillResourceClass` enum and a table-driven, case-sensitive `SkillResourceClassifier`; `safe_mode` validation raises the registry `ValidationError` for non-boolean values (including `null` and `1`); absence stays canonical for the Package ID while an explicit `true` is normalized away and `false` is retained, exactly per D-9/D-3. The pre-existing backslash gap is closed (validator, error message, tests, and docs all agree). `SkillLoader` needs no changes — the catalogue serializes `to_context_dict()`, which now carries `safe_mode` and per-resource `class`; the only consumer (`ToolAgent._skill_tool`) reads `content`/`encoding` keys, so the additions are inert. Tests are genuinely assertive: ID equality/difference is checked against an independently computed canonical ID, all five classes plus the `Scripts/` case-sensitivity rule are covered, and the loaded-catalogue exposure is verified in both Behave and Robot. Documentation is thorough and internally consistent (ADR D-8/D-9/D-10 + alternatives A-7..A-13 + revision history; standard 1.2.0 field table, classification paragraph, §16.2 identity paragraph, §16.3 example; new authoring guide wired into mkdocs nav; example package; CHANGELOG entry). Coverage gating and all required CI jobs pass. Only two documentation nits remain, neither blocking.
CoreRasurae force-pushed feature/m1-skill-safe-mode-resource-classification from ae7b44734a
Some checks failed
CI / lint (pull_request) Successful in 1m4s
CI / quality (pull_request) Successful in 1m25s
CI / typecheck (pull_request) Successful in 1m33s
CI / security (pull_request) Successful in 1m56s
CI / build (pull_request) Successful in 2m12s
CI / integration_tests (pull_request) Successful in 3m59s
CI / unit_tests (pull_request) Successful in 5m26s
CI / coverage (pull_request) Successful in 5m47s
CI / status-check (pull_request) Successful in 1s
CI / benchmark (pull_request) Failing after 18m43s
to c97ffe7b6b
Some checks failed
CI / lint (pull_request) Successful in 2m16s
CI / typecheck (pull_request) Successful in 1m52s
CI / build (pull_request) Successful in 1m29s
CI / quality (pull_request) Successful in 2m37s
CI / security (pull_request) Successful in 3m4s
CI / integration_tests (pull_request) Successful in 3m8s
CI / unit_tests (pull_request) Successful in 5m12s
CI / coverage (pull_request) Successful in 6m0s
CI / status-check (pull_request) Successful in 2s
CI / lint (push) Successful in 1m28s
CI / typecheck (push) Successful in 2m1s
CI / build (push) Successful in 1m40s
CI / quality (push) Successful in 2m8s
CI / security (push) Successful in 2m39s
CI / integration_tests (push) Successful in 3m44s
CI / benchmark (pull_request) Failing after 18m10s
CI / unit_tests (push) Successful in 6m14s
CI / coverage (push) Successful in 5m31s
CI / status-check (push) Successful in 2s
CI / benchmark (push) Failing after 19m3s
2026-11-10 13:43:46 +00:00
Compare
CoreRasurae deleted branch feature/m1-skill-safe-mode-resource-classification 2026-11-10 14:00:40 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
cleveragents/cleveractors-core!166
No description provided.