feat(skills): expose assets and gated non-standard resources through the skill tool #167
No reviewers
Labels
No labels
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Blocks
#162 feat(skills): expose assets and gated non-standard resources through the skill tool
cleveragents/cleveractors-core
Reference
cleveragents/cleveractors-core!167
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/m1-skill-resource-manifest-and-assets"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements the ADR-2034 D-10 read surface for Skill packages:
references/,assets/, andscripts/, with every entry tagged by its D-8 class.safe_mode: false). Reading such a path while either switch is safe fails with anExecutionErrornaming both states and the requested path.[SKILL_RESOURCE_READ]envelope plus ADR-2033 pagination are preserved.skilltool schema and the authoring/docs to describe the manifest, asset reads, and gated non-standard access.Closes #162
Scope
Script execution,
execute/args/timeout, and temporary-tree materialization remain the D-10 execution follow-up and are not included here.Verification
nox -s unit_tests— 3202 scenarios passednox -s integration_tests -- robot/agent_skills.robot— 9 tests passednox -s coverage_report -- features/agent_skills.feature— new manifest/gate paths fully coverednox -s lint,nox -s format -- --check,nox -s typecheck,nox -s security_scan,nox -s dead_codenox -s docs,nox -s benchmark -- --bench Skill34fc640b40ec7a3e3d60PR Review: !167 (Ticket #162)
Verdict: Approve
The implementation faithfully delivers the ADR-2034 D-10 read surface scoped by ticket #162: the class-grouped bundled-resource manifest on activation, asset reads with text-decoding/base64 fallback, the D-9 double-safety gate for non-standard reads (correctly refusing when either switch is safe, naming both states and the path), scripts readable in every mode, and the updated
skilltool schema plus docs. No critical or major issues were found; the three nits below are optional follow-ups and none block merge.Critical Issues
None.
Major Issues
None.
Minor Issues
None.
Nits
src/cleveractors/agents/tool.py:1158— gated paths leak into the "Available resources" error listing. While the D-9 gate is closed, a failed resource lookup lists every resource including non-standard entries — the same paths the activation manifest deliberately hides. ADR-2034 D-10 says the error names "the available resources", so this is compliant as written, but filteringnon_standardentries while the gate is closed would be consistent with D-9's hiding intent (a probing read currently reveals gated paths by name). Notedocs/tools/built-in-tools.mddocuments the listing behavior too, so this would need a small ADR/docs amendment if adopted.src/cleveractors/agents/tool.py:53—_SKILL_NON_STANDARD_CLASS = "non_standard"duplicatesSkillResourceClass.NON_STANDARD.value. If the enum value ever changed, the gate would silently fail open (non-standard reads allowed). The Behave gate matrix uses the real classifier, so drift would be caught by tests, but deriving the constant from the enum (or a parity assertion) is more robust than a string literal.features/agent_skills.feature:618— no scenario reads a root-level file. The D-8 fixture set createsLICENSE.txtand the ticket's expected behavior says root-level files remain readable in every mode, but no Behave/Robot scenario reads one. The read path is shared with reference reads so line coverage is unaffected; a one-line read scenario would complete the D-8 read matrix.Summary
Reviewed against ticket #162 and ADR-2034 (D-8/D-9/D-10), with the Package Registry Standard §16 and the skill docs as supporting context. Verification highlights:
references/,assets/,scripts/with class tags in ADR-mandated form and hides non-standard entries unless the gate is open; root files are correctly excluded from the manifest and readable in every mode. Non-standard reads raiseExecutionErrornaming both safety states and the path; the gate predicate (_is_global_unsafe= ToolAgentsafe_modefalse or context_unsafe_mode) exactly mirrorsLLMAgent._resolve_parent_unsafe, and the ephemeral tool agent is constructed withsafe_mode = not parent_unsafe(llm.py:1028), so end-to-end semantics are consistent. Script sources remain readable in every mode;execute/args/timeoutare correctly absent (deferred to #163, matching the PR's declared scope).skill.get("safe_mode", True) is not Falseis safe givenSkillValidator._validate_safe_modeenforces booleans; the manifest is appended to both pagination return branches on activation only; binary base64 resources fall back to the base64 string without crashing, matching the "no binary channel" ADR rationale. The previously-ungated non-standard reads are now intentionally tightened, documented in CHANGELOG and the guide.before_scenarioresets prevent cross-scenario leakage of the skills catalogue, safety flag, and binary fixture.Prior review comments: none existed at review time, so no previously-raised concerns needed re-checking. Script execution remains tracked by #163 and is deliberately out of scope here.
Nice work — approving. The nits are optional; address them in a follow-up if you agree with any.
@ -551,0 +615,4 @@"tools": [{"name": "skill"}]}"""And a skills context with skill "pdf-processing" having resources of every D-8 classNit: the new D-8 matrix fixtures include the root-level file
LICENSE.txt, but no scenario ever reads it. The ticket's expected behavior says root-level files remain readable in every mode; the read path is shared with reference reads so coverage isn't affected, but a one-line "Reading a root-level file from an activated skill" scenario would complete the D-8 read matrix.@ -45,0 +50,4 @@("asset", "assets"),("script", "scripts"),)_SKILL_NON_STANDARD_CLASS = "non_standard"Nit:
_SKILL_NON_STANDARD_CLASS = "non_standard"duplicatesSkillResourceClass.NON_STANDARD.valuefromskill_schema.py. If the enum value ever changed, the gate would silently fail open (non-standard reads allowed) — the Behave gate matrix would catch the drift, but deriving the constant from the enum (tool.py otherwise consumes plain data, so a module-levelSkillResourceClass.NON_STANDARD.valueimport or a parity assertion in tests) would be more robust.Nit: while the D-9 gate is closed, this "Available resources" listing includes non-standard entries (e.g.
fixtures/sample.pdf) — the same paths the activation manifest deliberately hides. ADR-2034 D-10 does say the error names the available resources, so this is spec-compliant as written; but filteringnon_standardentries from this listing while the gate is closed would be consistent with D-9's hiding intent (a probing read currently reveals gated paths by name). Would require a small ADR wording amendment if you choose to filter.ec7a3e3d60ead2669a44View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.