fix(skills): prevent private background review writes - #73373
Conversation
8ae2949 to
a9496ad
Compare
a9496ad to
0f1133b
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused privacy and provenance hardening. The underlying behavior is still present on current main: agent/background_review.py:181-205 promotes personal style/preferences into skill updates, and agent/background_review.py:219-227 permits session-specific reference material.
Problems
tools/skill_manager_tool.py:901rejects backup-style filenames through_validate_file_path(), but the same validator is used by_remove_file()attools/skill_manager_tool.py:1401. A legacyreferences/foo.md.bakcannot be removed throughskill_manage, so the package cannot be cleaned through the managed interface.tools/skill_manager_tool.py:114misses/mnt/c/Users/alice/...; its precedingcfails the negative lookbehind. This is a documented Hermes WSL path form inagent/prompt_builder.py:965-973, so it bypasses the proposed autonomous home-path hygiene check.
Suggested changes
- Reject artifact names on write/overwrite paths only, and add a removal regression.
- Add WSL-mounted Windows-home matching and a corresponding autonomous-write regression.
The PR is currently conflicting with main and its target test files were pruned after its base, so salvage will require conflict-aware test placement. Automated hermes-sweeper review.
|
|
||
| normalized = Path(file_path) | ||
|
|
||
| if _BACKUP_ARTIFACT_RE.search(normalized.name): |
There was a problem hiding this comment.
_validate_file_path() is also used by _remove_file() (line 1401), so this prevents removing an existing references/foo.md.bak. Apply this rejection only to creation/overwrite paths and add a regression proving legacy artifacts remain removable.
| r"(?P<domain>[A-Za-z0-9-]+(?:\.[A-Za-z0-9-]+)+)" | ||
| ) | ||
| _POSIX_HOME_RE = re.compile( | ||
| r"(?<![A-Za-z0-9])/(?:Users|home)/(?P<account>[^/\s`'\"<>]+)" |
There was a problem hiding this comment.
/mnt/c/Users/alice/... does not match because the c before /Users fails this lookbehind. Hermes documents that exact WSL path form in agent/prompt_builder.py:965-973; include it in the privacy matcher and test it.
What does this PR do?
Prevents autonomous background review from turning private, session-specific, transient, or unverified material into reusable skills.
The change corrects the root cause at both layers:
Nothing to save.is valid when no reusable procedure was verified.Related Issue
N/A — proactive privacy and provenance hardening after auditing autonomous skill-review behavior.
Type of Change
Changes Made
agent/background_review.py,agent/prompt_builder.py, andagent/learn_prompt.pyto separate reusable procedure from personal memory and transient session state.tools/skill_manager_tool.py.How to Test
scripts/run_tests.sh tests/run_agent/test_review_prompt_class_first.py tests/run_agent/test_background_review.py tests/tools/test_skill_manager_tool.py tests/tools/test_write_approval.py tests/agent/test_prompt_builder.py tests/agent/test_learn_prompt.py.388 tests passed, 0 failed.git diff --check.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — N/A; behavior is documented in the prompt policy and code docstringscli-config.yaml.exampleif I added/changed config keys — N/A; no config changesCONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/Askill_manageschema is unchangedScreenshots / Logs
Focused hermetic gate after rebasing onto current
origin/main:Changed-file Ruff and
git diff --checkboth pass. Cache-safety review is clean: only startup prompt constants changed; no conversation-loop mutation, role changes, toolset swaps, or pagination were introduced.The full repository wrapper was also run. It initially reported 43 failures across 18 files plus one timeout. Five were obsolete prompt-contract tests affected by this patch and were corrected; the remaining 38 failures across 17 untouched files are unrelated platform/concurrency baseline failures (including
/tmpversus/private/tmpassertions and Linuxsystemctlexpectations on macOS). The affected hermetic suite above is green.