Skip to content

fix(review): teach skill-review prompts the v2026.7.1 read-before-write guard #60331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kkwills13 wants to merge 1 commit into from
Closed

fix(review): teach skill-review prompts the v2026.7.1 read-before-write guard #60331

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions agent/background_review.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,10 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict]
" 1. UPDATE A CURRENTLY-LOADED SKILL. Look back through the "
"conversation for skills the user loaded via /skill-name or you "
"read via skill_view. If any of them covers the territory of the "
"new learning, PATCH that one first. It is the skill that was in "
"play, so it's the right one to extend.\n"
"new learning, PATCH that one first (after re-loading it with "
"skill_view in this turn — see the read-before-write rule "
"below). It is the skill that was in play, so it's the right one "
"to extend.\n"
" 2. UPDATE AN EXISTING UMBRELLA (via skills_list + skill_view). "
"If no loaded skill fits but an existing class-level skill does, "
"patch it. Add a subsection, a pitfall, or broaden a trigger.\n"
Expand Down Expand Up @@ -229,6 +231,16 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict]
"codename, library-alone name, or 'fix-X / debug-Y / audit-Z-today' "
"session artifact. If the proposed name only makes sense for "
"today's task, it's wrong — fall back to (1), (2), or (3).\n\n"
"Read-before-write (ENFORCED — skill_manage refuses otherwise): "
"before ANY patch/edit/delete of an existing skill, or "

@teknium1 teknium1 Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please scope this to existing-file mutations. write_file only enforces the read guard when the support-file target exists (tools/skill_manager_tool.py:1167-1172), so a new support file cannot be pre-read; _delete_skill also has no read-before-write guard. Exempt new skills/files and omit delete from this enforcement description.

"write_file/remove_file of its supporting files, load the exact "
"target in THIS review turn: skill_view(name) for SKILL.md, "
"skill_view(name, file_path=...) for a supporting file. Skill "
"content quoted earlier in the conversation does NOT satisfy "
"this. Call skill_view for the target, then issue the write in "
"the same reply or the next one, basing the new content on what "
"skill_view just returned. Touching multiple files? Handle them "
"one at a time: view, write, then move to the next.\n\n"
"User-preference embedding (important): when the user expressed a "
"style/format/workflow preference, the update belongs in the "
"SKILL.md body, not just in memory. Memory captures 'who the user "
Expand Down Expand Up @@ -298,8 +310,9 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict]
"Preference order for skills — pick the earliest that fits:\n"
" 1. UPDATE A CURRENTLY-LOADED SKILL. Check what skills were "
"loaded via /skill-name or skill_view in the conversation. If one "
"of them covers the learning, PATCH it first. It was in play; "
"it's the right place.\n"
"of them covers the learning, PATCH it first (after re-loading "
"it with skill_view in this turn — see the read-before-write "
"rule below). It was in play; it's the right place.\n"
" 2. UPDATE AN EXISTING UMBRELLA (skills_list + skill_view to "
"find the right one). Patch it.\n"
" 3. ADD A SUPPORT FILE under an existing umbrella via "
Expand All @@ -316,6 +329,16 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict]
"codename, library-alone name, or 'fix-X / debug-Y' session "
"artifact. If the name only fits today's task, fall back to (1), "
"(2), or (3).\n\n"
"Read-before-write (ENFORCED — skill_manage refuses otherwise): "
"before ANY patch/edit/delete of an existing skill, or "
"write_file/remove_file of its supporting files, load the exact "
"target in THIS review turn: skill_view(name) for SKILL.md, "
"skill_view(name, file_path=...) for a supporting file. Skill "
"content quoted earlier in the conversation does NOT satisfy "
"this. Call skill_view for the target, then issue the write in "
"the same reply or the next one, basing the new content on what "
"skill_view just returned. Touching multiple files? Handle them "
"one at a time: view, write, then move to the next.\n\n"
"User-preference embedding: when the user complains about how "
"you handled a task, update the skill that governs that task — "
"memory alone isn't enough. Memory says 'who the user is and "
Expand Down