The pre-existed branch of _restore_snapshot unlinked only SKILL.md, so a batch
[create gamma (adopting an empty leftover), write_file gamma references/a.md,
failing op] left references/a.md behind while reporting "all touched skills
rolled back" — and the next create was refused as occupied, wedging the user
until they hand-deleted the dir. The batch already records each applied op's
name/file_path; hand those to the rollback so it unlinks exactly what the batch
wrote and rmdir()s the emptied dirs up to the skill dir. rmdir() fails on
anything else, so a file that landed out-of-band still survives.
One ownership rule for an adopted empty dir on both paths: the single-op create
also always rmdir()s after a blocked scan instead of tracking a created_dir flag
(rmdir removes only an empty dir, so the flag added nothing but a second rule).
create_targets loses its never-used default; _create_skill reuses
mkdir_under_hermes_home for the parent instead of inlining its assert + mkdir.
_skill_manage_batch resolved every create op's target dir (base / category / name)
before anything checked the category, so a non-string category raised TypeError
out of skill_manage instead of the JSON error the single-op path returns — and it
did so after mkdtemp, leaking the skill_batch_ snapshot dir. Reject it in
_validate_batch_ops with the same _validate_category the single-op path uses, so
the batch fails pre-effect like every other shape error.
_snapshot_skills only sees a SKILL.md, so an empty pre-existing directory that
create adopts left snap=None and a later op failure rmtree()'d the whole dir,
including anything dropped into it mid-batch (ehz0ah's review repro on #120437).
Record for each create target whether the dir already existed; on rollback undo
only the SKILL.md the batch wrote and rmdir if that leaves it empty, mirroring the
single-create path which rmdir()s only a dir it made.
Since operations[] became the only call shape (#97295) the batch's success
path rebuilt each row as {name, action, file_path, success} and dropped every
advisory key the per-op handler attached — lint_warnings/lint_hint from the
create linter and the org_sharing note. The model never saw a finding.
Carry those keys onto the row, mirroring what the failure path already does
for teaching payloads.
The per-action schema made the delete branch `additionalProperties: false`
with no `absorbed_into`, so a schema-validating or grammar-constrained
backend could no longer emit the curator's consolidation delete and
`_curator_consolidation_delete_guard` fail-closed every consolidation.
Advertise `absorbed_into` on the delete branch and cover the batch path
that forwards it to the guard.
Also fold the two remaining patch shape checks (missing new_string,
content mixed with old_string/new_string) into `_op_shape_error`, so a
batch rejects them before applying any sibling instead of creating op[0]
and rolling it back. Drop the stale `edit` vocabulary from skills.md:440
and the curator prompt, and move the schema-diet test helpers above the
`__main__` guard.
The operations[] item schema was one flat object with four coexisting text
slots (content / new_string / file_content / file_path). A 27B local model
that had just used write_file's file_content kept emitting it on create and
patch ops; the call validated against the advertised schema, the handler
failed on "content is required", and the whole batch rolled back — eight
identical retries until the tool-loop guardrail tripped (#112677).
- items is now an anyOf of self-contained per-action op objects (create,
patch targeted, patch full-rewrite, write_file, remove_file, delete), each
with additionalProperties: false. The wire shape of a correct call is
unchanged (still a flat op with name/action/...), so transcripts, staging
and replay are untouched; grammar-constrained backends can no longer emit
another action's slot, and schema-validating providers reject it up front.
Nested (non-top-level) anyOf survives every sanitizer (schema_sanitizer,
Gemini legacy translator). Cost: parameters JSON 1352 -> 2414 bytes.
- _validate_batch_ops runs the per-op argument-shape check (_op_shape_error)
before any op is applied, so a misfiled op[1] no longer applies op[0] and
then rolls the batch back; the error carries the same misplaced-key hint.
- The hint is attached to argument-shape misses only: a patch whose real
problem is an unmatched old_string is no longer told to "move that text to
'content' (full rewrite)", the escape the patch error itself warns against.
- Shape tables (_REQUIRED_ARGS, text-slot maps, _misplaced_text_hint,
_op_shape_error) move out of the facade into skill_manager_batch.py, the
op-validation sibling; _patch_skill shares the old_string guidance text.
- Docs: skills.md Actions table states the one-slot-per-action contract.
Slim follow-up to the cherry-picked #111585 (@KoNit-K):
- tools/skill_usage.py: generalize the usage ledger's `_usage_file_lock()` into
`skill_file_lock(lock_path)` — same fcntl/msvcrt idiom, now thread-re-entrant
via a per-thread held set (flock is not re-entrant across separate fds; a
ContextVar would leak "held" into copy_context() timer threads).
- tools/skill_manager_tool.py: drop the third fcntl/msvcrt copy, hashlib and the
ContextVar; the per-skill lock is `<skills>/.locks/<skill-dir-name>.lock`
(readable, outside the skill dir so delete/recreate cannot unlink it under a
waiting writer). Batch locks sort by lock PATH, not name, so two batches
naming the same skills in different forms cannot deadlock.
- tools/skill_manager_batch.py: plain `with` around snapshot -> commit/rollback
instead of manual __enter__/__exit__ bookkeeping.
- tests: trimmed to two invariants — the two-writer lost-update test on
SKILL.md (from #111585) and a re-entrancy/exclusivity test on the helper.
Dropped: the edit/write_file/remove_file parametrization (same dispatcher
path as patch) and the category-dir cleanup test (lock files never lived in
category dirs here).
For each issue anchor present in BASE 63279301bc non-test .py and absent on HEAD, the BASE comment/docstring block was re-attached at the HEAD location of the code it explained (matched by the distinctive code line / enclosing def). Sentences already covered by an existing HEAD comment were deduped; the issue number always survives. Insert-only: no code lines changed.
