Repository navigation
fix(skills): treat only top-level scripts/ paths as executable in skill_manage - #7088
Conversation
github-actions
Bot
added
area:backend
3fb2ee9 to
3a35f4e
Compare
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Verified the fix: Path(path).parts[:1] == ("scripts",) matches the scripts/-member clause of is_code_path in skills/package_files.py, and I traced the edge shapes. ./scripts/x, scripts//x, and scripts/sub/x stay executable; bare scripts, //scripts/x, Scripts/, and backslash spellings never reach the gate because ensure_safe_support_path rejects them first (absolute path, single segment, top-level dir not in the allowed set), so the host-native Path here agrees with the storage parse on both POSIX and Windows hosts. Releasing nested references/scripts/... and templates/my-scripts/... writes to the static tier is sound: the deterministic SkillScan still runs over the whole candidate dir and still classifies code-suffixed support files as code via is_code_file, and the publication path (mutations/scanner.CandidateScanner) re-classifies with is_code_file and fails closed, so the relaxation does not recreate a hole downstream. No twin unfixed instances found: the installer and the mutations scanner already consume the shared package_files rule. Two non-blocking notes below.
| # the storage layer already parsed this path with the same Path | ||
| # semantics. A substring test here also caught support files such as | ||
| # references/scripts/overview.md and templates/my-scripts/setup.md. | ||
| executable = Path(path).parts[:1] == ("scripts",) |
There was a problem hiding this comment.
Suggestion: this re-derives the scripts/-member rule locally, which the skills module's AGENTS.md explicitly says not to do (package_files.py is the single definition of code-file rules; the installer, export guard, and SkillScan all import it). For every path that actually reaches this line the predicate does agree with the shared rule, since ensure_safe_support_path has already rejected absolute paths, traversal, the single-segment scripts spelling, and (on POSIX) backslash forms, and Path parses with the same semantics the storage check used. But package_files._posix additionally normalizes backslashes to /, so the two derivations are not equivalent in general. A tiny shared helper in package_files.py (e.g. is_scripts_member(path) built on _posix) would keep this gate, the installer, and SkillScan in lockstep if the member rule ever evolves.
| "write_file", | ||
| "demo-skill", | ||
| content, | ||
| "scripts/run.sh", |
There was a problem hiding this comment.
Test-boundary nit: the positive case pins only a direct scripts/run.sh child. Two cheap additions would pin the anchoring from the remaining sides: scripts/sub/run.sh (a file nested under the top-level scripts/ dir must stay executable=True — the half of the old substring behavior the new predicate intentionally keeps) and the ./scripts/run.sh spelling (Path normalizes the leading . away, so it stays executable). Both follow from the predicate today, but they are exactly the shapes a future refactor to PurePosixPath or manual split('/') could regress.
github-actions
Bot
added
the
area:skills
|
Both non-blocking notes are addressed in
Validation on
Current head: |
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed 0cc106e8b84182207e472f3a8dbbef5902df89a8. Both earlier suggestions are addressed: the directory-membership rule is shared, and nested/dot-prefixed script paths have coverage. One P2 moderation regression remains, detailed inline.
Correction to my earlier review: I incorrectly stated that CandidateScanner supplies a downstream executable-content check for this tool's publication path. skill_manage writes directly through write_custom_skill; that scanner does not run here.
Validation: 325 related tests passed. Eight additional offline policy regressions fail at this head and pass with the exact base tool implementation. A temporary in-memory change to the complete shared is_code_file classifier passes those eight probes plus all 11 PR regression cases (19 passed). Lint and format checks passed; production source files were unchanged.
| # of re-deriving it; a substring test here also caught support files | ||
| # such as references/scripts/overview.md and | ||
| # templates/my-scripts/setup.md. | ||
| executable = is_scripts_member(path) |
There was a problem hiding this comment.
[P2] Preserve executable moderation for nested code files
is_scripts_member(path) also downgrades runnable support files such as references/scripts/helper.py, templates/my-scripts/run.sh, assets/my-scripts/tool.js, and extensionless shebang files under references/scripts/. These are code according to the shared is_code_file rule and previously received executable=True, but now get written when moderation returns warn, or when the provider is unavailable with security_fail_closed=false.
I reproduced both outcomes through the real tool and storage with the deterministic package scan retained. That scan does not enforce the allow-only model verdict, and the following write_custom_skill call does not invoke CandidateScanner, so no later check restores executable moderation. All eight policy probes reject the writes on the base revision and accept them at this head.
Please classify with the complete shared code-file rule, including shebang detection, and add warn/unavailable regressions while keeping ordinary Markdown support files on the static tier.
…ll_manage
write_file classified a support file as executable content with a substring
test ("scripts/" in path), so any support path merely containing "scripts/"
(e.g. references/scripts/overview.md or templates/my-scripts/setup.md) was
moderated as executable: that scan requires an exact "allow" decision and
hard-blocks when the moderation model is unavailable. Executable content is
now exactly a file under the skill's top-level scripts/ directory, matching
the scripts/ member rule in skills/package_files.py and the same Path
semantics the storage layer already used to validate the support path.
The write_file executable gate re-derived the top-level scripts/ member rule locally. Use package_files.is_scripts_member instead so the gate, installer, and SkillScan stay in lockstep. Extend the regression test to pin scripts/sub/run.sh and ./scripts/run.sh as executable content, plus the shared helper's normalization.
…l_manage The write_file executable gate classified only top-level <scripts/> members as code, so runnable support files elsewhere in the package (references/scripts/helper.py, templates/my-scripts/run.sh, assets/my-scripts/tool.js) and extensionless shebang files fell back to static moderation: a warn decision, or an unavailable moderation model with security_fail_closed=false, wrote them to disk. The deterministic package scan does not enforce the allow-only model verdict, and this write path publishes through write_custom_skill without running the mutations CandidateScanner, so no later check restores executable moderation. Classify with the complete shared code-file rule from deerflow/skills/package_files.py (scripts members, code suffixes, and extensionless shebang files) instead of the scripts-member rule alone, and keep Markdown/data support files on the static tier. Extend the regression suite with the code-suffix/ shebang cases plus the warn and unavailable-provider outcomes.
0cc106e to
46c6c16
Compare
github-actions
Bot
added
size/L
|
P2 confirmed — thanks. The executable = is_code_file(path, content[:8].encode("utf-8", "surrogatepass"))
Effect on the paths you listed: New head: Measured on this head with
|
Why
A
skill_managewrite_filecall for a legitimate support file was moderated as executable content whenever the path merely contained the substring"scripts/"anywhere — for examplereferences/scripts/overview.mdortemplates/my-scripts/setup.md. Both are ordinary static support paths:ensure_safe_support_pathallows any file underreferences/,templates/,assets/, orscripts/, and only the skill's top-levelscripts/directory is executable content according to the package rule inskills/package_files.py(is_code_path: ascripts/member isposix.parts[0] == "scripts").The pain is over-blocking: the executable gate in
_scan_or_raiseis much stricter than the static one. For executable content the moderation scan must return an exact"allow"decision (a"warn"raisesSecurity scan rejected executable content), the moderation prompt is told the file is executable, and when the moderation model is unavailable the write hard-blocks withSecurity scan unavailable for executable content— even on deployments configured to fail open for ordinary skill content. No write escapes moderation; the misrouted write just receives the wrong (strictly stricter) tier than its path was supposed to get, so writing an innocuous reference document could be rejected.Root cause:
backend/packages/harness/deerflow/tools/skill_manage_tool.pyclassified the write withThe second operand is redundant, and the first is a plain substring test, so any support path containing
"scripts/"— including inside other directory names — was misclassified as executable.What changed
skill_managewrite_filenow classifies a support file with the complete shared package rule,deerflow.skills.package_files.is_code_file(path, head): executable content is a file under the skill's top-levelscripts/directory, a code-suffixed file anywhere in the package, or an extensionless file whose content starts with a shebang. Everything else is static support content and gets the ordinary moderation.That fixes both misclassifications, not just one:
references/scripts/overview.md,templates/my-scripts/setup.mdand other Markdown/data files under ascripts/-looking directory are static content again;scripts/-member rule alone left on the static tier —references/scripts/helper.py,templates/my-scripts/run.sh,assets/my-scripts/tool.js, extensionless shebang scripts such asreferences/scripts/hook— are executable content again, so awarndecision rejects them and an unavailable moderation model blocks them even withskill_evolution.security_fail_closed: false.Nothing else changes: the deterministic SkillScan still runs for every
write_file, top-levelscripts/...files keep the strict executable gate, andSKILL.mdwrites (create/patch/edit) stay on the static tier.The rule lives only in
skills/package_files.py:is_code_path()now reuses a smallis_scripts_member()helper, and the tool importsis_code_fileinstead of re-deriving ascripts/test. That is the same predicateskills/mutations/scanner.pyandskills/skillscan/orchestrator.pyconsume, so the write gate, the installer/export guard and SkillScan can no longer disagree. The shebang sniff reuses the first bytes of the content being written, so no extra file read is introduced, and Windowsscripts\separators are normalized by the shared_posixhelper.Behavior note: a code-suffixed or shebang file placed outside
scripts/(for examplereferences/tools/probe.py) is now executable content where the old substring test left it static. That is the stricter direction and the rule the rest of the package pipeline already applies; Markdown, JSON/YAML and other data files are unaffected.Surface area
frontend/backend/applanggraph.json, or prompt changedocker/or sandboxed executionskills/(the backend skills-management tool,packages/harness/deerflow/tools/skill_manage_tool.py, and the shared package-file rule inpackages/harness/deerflow/skills/package_files.py)backend/pyproject.tomlorfrontend/package.jsonBug fix verification
backend/tests/test_skill_manage_write_file_executable_gate.py(41 cases, both directions pinned):test_write_file_support_path_containing_scripts_substring_is_scanned_as_static—references/scripts/overview.md,templates/my-scripts/setup.mdmust be moderated asexecutable=False, including on awarndecision and on an unavailable provider undersecurity_fail_closed: false;test_write_file_non_code_support_file_stays_static— Markdown,references/schema.jsonand an extensionless non-shebang file stay writable in both degraded situations;test_write_file_code_support_file_outside_top_level_scripts_is_scanned_as_executable— the review-named runnable files (references/scripts/helper.py,templates/my-scripts/run.sh,assets/my-scripts/tool.js, shebangreferences/scripts/hook) must reach moderation as executable content;test_write_file_rejects_code_support_file_when_moderation_warnsandtest_write_file_rejects_code_support_file_when_moderation_unavailable_and_fail_open— those files must not be written onwarn, nor when the provider is unavailable even withsecurity_fail_closed: false;test_write_file_top_level_scripts_file_is_scanned_as_executable,test_is_scripts_member_uses_the_shared_posix_rule,test_is_code_file_covers_code_suffixes_and_shebangs_anywhere_in_the_package— unchanged top-levelscripts/behavior and the shared rule itself.mainand green on this branch? Yes.main:17 failed, 24 passed— every static-tier case recordsexecutable=True, and a code file outsidescripts/recordsexecutable=False.41 passed.Validation
Ran from the repo root with the frozen backend environment (
uv run --frozen --project backend ...):python -m pytest backend/tests/test_skill_manage_write_file_executable_gate.py -q→ red onmain(17 failed, 24 passed), green on this branch (41 passed)python -m pytest backend/tests/test_skill_manage_write_file_executable_gate.py backend/tests/test_skill_manage_tool.py backend/tests/test_skill_storage_support_files.py backend/tests/test_skill_package_paths.py backend/tests/test_security_scanner.py backend/tests/test_skill_export.py backend/tests/test_skills_installer.py backend/tests/test_skill_mutation_scanner.py -q→ 269 passed, 42 skipped (the same battery is 228 passed, 42 skipped onmain)warn, unavailable-provider fail-open over the same paths plusreferences/tools/probe.py) → 33 passed, and 151 passed when run together withtest_skill_manage_tool.py+test_skills_installer.pypython -m ruff checkandpython -m ruff format --checkon both changed files → cleanAI assistance
Tool(s) used: Claude Code
How you used it: AI located the misclassification, wrote the regression tests (red on
main) and the classification fix. Review follow-ups moved thescripts/-member rule into the sharedpackage_fileshelper and then replaced it with the complete sharedis_code_filerule plus warn/unavailable regressions; every test result above was actually executed againstmainand this branch.