Add put-file and rm-file for single-path skill edits - #62
Merged
Conversation
Change one file in a hosted skill without resending the rest of its tree:
goodeye skills put-file <skill> <path> --from-file ./notes.md
goodeye skills rm-file <skill> <path>
Paths the command does not name ride forward untouched, unlike
`goodeye skills publish <dir>`, which uploads a whole-tree snapshot where
any omitted path is deleted. A file's executable mark and role label keep
their current values unless the caller sets them, so a flag that was not
passed is left off the wire rather than sent as a default that would reset
a setting the caller never mentioned.
Text and binary content pick their wire field through one shared decision,
now used by both the whole-tree snapshot builder and these commands, so the
two can never disagree about whether a given file is text.
With no --expected-version-token, the current token is read first and passed.
That is not a weaker guard: the server still rejects a writer who landed
between the read and the write, which is the only race a single command
invocation has. The flag stays for scripts that already hold a token.
After a successful change, the local sync index records the new state for
that path (or drops it, for a removal) in every target mirroring the skill.
Without that, the recorded hash would stay stale and the next
`goodeye skills sync push` would report drift for a file the registry
already holds, so the commands would introduce false drift on their own.
When no target tracks the skill, the index is left untouched.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The per-file refresh after put-file and rm-file updated every mirror of the skill while advancing the recorded version on none of them, leaving each index claiming content from the new version and a sync point at the old one. The next push then sent a token the server had already replaced, was told it conflicted, and pointed the user at a pull that refuses whenever there is also a local edit, so the only way forward was a forced pull that discards that edit. No second writer was involved: the conflict was manufactured by the index. The refresh now applies one predicate. A mirror is updated only when its recorded version token matches the one the change was written against, and such a mirror moves its file state and its sync point together onto the version the change produced. Mirrors recorded at any other version are left for a pull. Two smaller fixes ride along. A path absent from the recorded manifest is no longer recorded at all: the response carries no per-file metadata, and invented executable and purpose values would be read back by the next full push and sent, clearing what the server holds. And the recorded hash for a SKILL.md write now applies the same newline translation that reading the body back applies, so a runbook with CRLF line endings no longer reports drift that can never clear. Also strengthens the help-text tests to assert the contrast with publish rather than the bare word, and covers the four server error statuses for both commands.
The refresh advanced the recorded manifest and the sync point but left the directory those describe untouched, so the index claimed content the mirror did not hold. Every route out of that state loses the change. The push builds its snapshot from the directory, so a stale copy is sent straight back: after rm-file the removed file is uploaded again and the removal is undone, and after a put-file whose content came from anywhere other than the mirror the old bytes overwrite the new ones. The pull that would repair it sees a sync point already at the version the change produced and either reports the mirror up to date or, once the directory counts as locally modified, refuses it, so neither surface reconciles. The refresh now moves the directory with the index. A mirror updated by the change has the bytes written into it, or the removed path taken out of it, at the same time as its recorded file state and sync point; a mirror recorded at another version is still left for a pull. The mark applied to the local copy is the one the caller passed, else the one already recorded, and nothing is written where the skill has no directory under the target: that mirror was never materialized and the pull path fetches it whole. Covers the three cases the previous shape got wrong: a put-file sourced from outside the mirror, the same for the runbook, and an rm-file, each asserted to leave the directory correct and the next push with nothing to do.
A path the local manifest never held was left out of it, so the next full push both reported drift for a file already current and sent it with no label, clearing server-side the very label the caller had just set: the push is a full snapshot and reads labels only out of the index. It is now recorded whenever the caller passed a mark or a label, where nothing has to be invented. The value passed is what the registry holds, and the other one is read off the local copy the write just made, which is what the next push would read there anyway. A path named with neither still stays out, and a path with no local copy is never recorded, so the manifest never describes content its directory does not hold.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds two commands for changing one file in a hosted skill without resending the rest of its tree.
Both call
PATCH /v1/skills/{id_or_slug}/files. Paths the command does not name ride forward untouched, which is the opposite ofgoodeye skills publish <dir>: that uploads a whole-tree snapshot, so any path missing from the directory is deleted. The--helpfor both commands carries that contrast.Behavior worth reviewing
Flags the caller did not pass are not sent. On this route an absent
executableorpurposemeans "keep the file's current value", so sending a default would silently reset a label the caller never mentioned.--executableand--no-executableare distinct from omitting both.One decision for text vs. binary.
inline_content_fieldis now the single place that pickscontent(verbatim UTF-8 text) orcontent_base64(base64 bytes), used by both the whole-tree snapshot builder and these commands, so the two can never disagree. Behavior is unchanged from what the snapshot builder already did: invalid UTF-8 or an embedded NUL is binary, and text that happens to look like base64 (test) still goes throughcontent.Token resolution. With
--expected-version-tokenomitted, the current token is read first and passed. That is not a weaker guard: the server still rejects a writer who landed between the read and the write, which is the only race a single command invocation has. The flag stays for scripts that already hold a token, and skips the read.Local sync index refresh. After a change lands, the index records the new hash (plus the executable bit and purpose label) for that path, or drops the entry for a removal, in every target mirroring the skill. Without this the recorded hash stays stale and the next
goodeye skills sync pushreports drift for a file the registry already holds, so the commands would introduce false drift on their own. When no target tracks the skill, the index is left untouched.SKILL.mdupdates the recorded body hash instead of entering the file manifest, since the on-disk walk never yields it and recording it there would manufacture permanent drift. The refresh is best-effort: the write already succeeded, so an unreadable index prints a note pointing atsync pullrather than failing the command.Tests
Written first and confirmed failing before the implementation. 26 new tests cover both content sources and rejecting both at once, a missing local file, the text/binary channel asserted equal to the snapshot builder's choice for the same bytes, the carry-forward flags on the wire, token resolution with and without the flag, and the sync-index refresh in both directions: tracked (single target, several targets, a path new to the manifest, a removal,
SKILL.md) and untracked (index file byte-identical afterward). One end-to-end test edits a file inside a sync target, sends it, and assertstree_push_driftedis false afterward.Version bumped to 0.25.2. No release tag: that is a separate operator-run step.
Verification
uv sync --group dev --frozen && uv run pre-commit run --all-files && uv run pytest(the CI workflow's three steps): ruff, ruff format, and pyright pass; 1090 tests pass, up from 1064 on the branch point.🤖 Generated with Claude Code